Repository navigation
[aw] Add trusted KBE search eval support - #133958
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues remain in grader matching, global grader wiring, and production-coupled test setup.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
.github/workflows/evals/kbe-candidate-reads-grader.mjs:130
- A tool result can carry
success: truetogether with an{ isError: true }payload; the existing eval grader explicitly rejects that shape. This branch records such a failed read as successful because it only checks the transport flag and the[Filtered]text, allowing a candidate to pass without a usable issue read. Reject result-level errors before adding the number toreads.
} else if (call.validScope && Number.isInteger(call.number) &&
event.data.success && !resultText(event.data.result).includes("[Filtered]")) {
.github/workflows/evals/kbe-candidate-reads-grader.mjs:131
- The temporal check stores the
tool_resultindex, not theissue_readcall index. For an interleaving where a read is started before the search result but completes afterward,reads.get(number)is aftersearchIndexand the pre-search read is accepted, contrary to the temporal test and the grader's intended enforcement. Record the read call's event index and compare that index with the search-result index.
reads.set(call.number, index);
.github/workflows/evals/kbe-candidate-reads-grader.mjs:144
- If the harness tool call has no matching
tool_result, it is counted inharnessCallCountbut never contributes an error or candidate. A trajectory that ends after an aborted/timed-out search can therefore reachpassed === truewith zero candidates; treat every unmatched search call as a failure before computing the result so this grader remains fail-closed.
const missing = [...candidates]
.filter(([number, searchIndex]) => !reads.has(number) || reads.get(number) < searchIndex)
.map(([number]) => number)
.sort((left, right) => left - right);
const passed = errors.length === 0 && missing.length === 0;
.github/workflows/evals/kbe-search.test.mjs:41
- This test only regex-matches the extracted production script for
execFile, the endpoint, and a token; it never executes that script. Because the production implementation is duplicated insideci-failure-scan.md, regressions in repository scoping, candidate validation, or output shape can pass while all behavioral tests exercise onlysearch-kbe-issues.cjs. Execute the extracted script with a mockedghtransport, or assert the full production contract.
test("production wrapper uses authenticated gh api transport", async () => {
const script = await productionScript();
assert.match(script, /execFile\)\("gh"/);
assert.match(script, /"api",\s*"search\/issues"/);
assert.match(script, /GITHUB_TOKEN/);
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
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.
🟡 Changes recommended
The critical trust-boundary findings must be fixed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Two CI-eval wiring issues and one grader command-validation issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
PureWeen
left a comment
There was a problem hiding this comment.
Holistic Review
Motivation: Providing a trusted KBE-search helper and reusable read-evidence grader is valuable groundwork for deterministic scan evaluations.
Approach: The prerequisite is now independently packaged and lintable, and it fixes the earlier path and canonical tool-name issues. The grader still cannot recognize the result envelope emitted by the pinned GitHub MCP SDK.
Summary: Needs Changes at 54e2f5a. One new merge-blocking result-decoding defect is reproduced below. The existing command-suffix bypass remains valid and is not duplicated here.
Detailed Findings
❌ Error — Decode the SDK issue-read envelope before extracting the issue number
.github/workflows/evals/kbe-candidate-reads-grader.mjs:29-37 only checks result.number and result.issue.number. Pinned @modelcontextprotocol/sdk 1.0.9 returns successful tool data under result.content (with optional structuredContent), and Vally passes that result through to the grader. A successful SDK-shaped github-issue_read for a real candidate therefore reproduces Missing successful issue_read, while the tests pass because they fabricate an unwrapped { number } object.
Decode the actual MCP result envelope before extracting the issue number, including the JSON text content shape and structuredContent when present. Add an adapter-contract regression fixture shaped like the pinned SDK result rather than only the simplified raw object.
Verified on this head: 13/13 checked-in prerequisite tests pass, all three specs strict-lint with pinned Vally 0.14, the canonical github-issue_read name is recognized, and both the absolute and spec-relative plugin paths load. No full live agent eval or Actions run was performed; CI status and merge readiness were not assessed.
Note
This review was generated by GitHub Copilot.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed in local commit Note This reply was generated by GitHub Copilot. |
There was a problem hiding this comment.
🟢 Approval recommended
The trusted helper/grader/tests are correctly isolated to the trusted temp directory, are wired in with conservative gating, and the added focused suite exercises the intended behaviors.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
PureWeen
left a comment
There was a problem hiding this comment.
Approving at d6a28918. The SDK result-envelope issue from my earlier review is fixed, the prerequisite tests and strict Vally lint pass, and the new grader stays inactive until a spec selects it.
Known follow-up: the command-suffix bypass in kbe-candidate-reads-grader.mjs (for example node "<helper>" ; printf '[]') still lets an agent satisfy the search check without a real query. That weakens eval signal rather than production behavior, and I agree it is reasonable to handle separately.
Note
This review 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
Role in the stacked change
This is the prerequisite for dotnet/runtime#133684. It must merge before #133684 so the trusted evaluator files are available from
main. The/ci-evalworkflow restores evaluator inputs frommainbefore checking out a pull request; without these files onmain, the dependent PR's wrapper, grader, and focused tests cannot be trusted or exercised by the evaluator.After this PR merges, #133684 can retain the production workflow, instruction, and eval-spec changes that consume this support.
Validation
npm test --prefix .github/workflows/evals— 13 tests passed.git diff --checkpassed.Note
This pull request description was generated by GitHub Copilot.