[aw] Make CI KBE issue search deterministic - #133684
vitek-karas wants to merge 2 commits into
Conversation
|
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
Unresolved issues affect fail-closed search behavior, wrapper enforcement, candidate verification, and workflow policy consistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates CI KBE issue searches to use a repository-scoped wrapper and require candidate inspection before duplicate decisions.
Changes:
- Adds deterministic issue-search validation.
- Updates prompts, MCP configuration, and evaluation checks.
- Regenerates the compiled workflow.
File summaries
| File | Reviewed changes |
|---|---|
.github/workflows/shared/create-kbe.instructions.md |
Updates candidate inspection and fail-closed guidance. |
.github/workflows/evals/search-kbe-issues.cjs |
Adds the scoped issue-search wrapper. |
.github/workflows/evals/README.md |
Documents updated eval transport usage. |
.github/workflows/evals/ci-failure-scan.eval.yaml |
Enforces wrapper and candidate-read usage. |
.github/workflows/ci-failure-scan.md |
Configures production tools and the search wrapper. |
.github/workflows/ci-failure-scan.lock.yml |
Regenerates the compiled workflow artifact. |
.github/workflows/ci-eval.yml |
Preserves trusted evaluation inputs. |
Review details
Suppressed comments (4)
.github/workflows/ci-failure-scan.lock.yml:1425
- This generated-only change switches agent-failure issue expiry from
0to 168 hours, while the other agentic workflow lock files still use0and no source setting in this PR requests the behavior change. That can hide an unresolved workflow failure after seven days; please restore the established value or add an explicit source-level setting and document the intended policy.
GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "168"
.github/workflows/evals/ci-failure-scan.eval.yaml:124
- The deterministic
tool-callsgrader only requires one matchingissue_read; it does not relate the number of reads to the candidates returned by the two wrapper calls. A run can read one issue, skip other candidate numbers (or fail a read), and still satisfy this grader; the prompt judge is not a deterministic fail-closed check. Add a post-run verifier or stateful harness that requires a successful read for every returned number before accepting output.
- name: 'issue_read'
args:
owner: '^dotnet$'
repo: '^runtime$'
method: '^get$'
.github/workflows/evals/search-kbe-issues.cjs:3
- This adds a second implementation of the wrapper: the eval executes
search-kbe-issues.cjs, while production executes the separate inlinemcp-scriptsblock inci-failure-scan.md. A future scope or validation change in only one copy can make the eval pass while production behaves differently. Factor the fetch/validation logic into one checked-in module and have both entry points call it, or generate the eval harness from the same source.
async function searchKbeIssues(query, token, fetchImpl = fetch) {
.github/workflows/shared/create-kbe.instructions.md:196
- These searches now require reading every candidate, but they do not define what happens when a read fails or returns
[Filtered]. The fail-closed rule above is scoped to KBE-oriented searches (and variation 5), so an unreadable plain tracker can be treated as a miss and allow a duplicate KBE. Add an explicit stop/skipped: integrity-filtered candidate, needs human reviewrule here.
Search for a plain tracker, then read every returned candidate:
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Critical grader integration and candidate-read enforcement issues, plus moderate workflow and parity issues, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
.github/workflows/ci-eval.yml:351
- The eval invocation repeats the same incorrect plugin path: this step's working directory is the repository root, while the trusted grader is restored under
.github/workflows/evals/. As a result the scanner eval cannot loadkbe-candidate-reads; use the restored path here as well.
grader_args+=(--grader-plugin ./kbe-candidate-reads-grader.mjs)
.github/workflows/ci-failure-scan.lock.yml:663
- The quoted heredoc writes
${RUNNER_TEMP}literally intotools.json(and the generated server below), rather than expanding it. The server's log path therefore does not match the actual${RUNNER_TEMP}/gh-aw/mcp-scripts/logsdirectory consumed by the parse/upload steps, so MCP-script diagnostics will be lost; generate this path fromprocess.env.RUNNER_TEMPor otherwise expand it before regenerating the lock.
"logDir": "${RUNNER_TEMP}/gh-aw/mcp-scripts/logs",
.github/workflows/ci-failure-scan.md:77
- Appending
repo:dotnet/runtime is:issuedoes not enforce the wrapper's scope: a caller can supply a conflicting or negated qualifier such as-repo:dotnet/runtimeoris:pr, producing an empty result while the wrapper reports success. The scanner could then treat an existing KBE as absent and file a duplicate. Reject or normalize conflicting caller qualifiers before invokinggh, and keep the same guard in the eval copy and generated lock implementation.
`q=${query.trim()} repo:dotnet/runtime is:issue`,
.github/workflows/ci-failure-scan.md:93
- The parity test only checks that this extracted script mentions
execFile,search/issues, andGITHUB_TOKEN; it never executes or asserts theincomplete_resultsand candidate-validation branches here. A future drift in the production wrapper could therefore reintroduce fail-open or malformed-candidate behavior while the trusted tests still pass. Add behavior-level parity coverage for these checks, or share one implementation.
if (result.incomplete_results !== false || !Array.isArray(result.items)) {
throw new Error("GitHub issue search returned an invalid response");
}
return result.items.map((item) => {
.github/workflows/evals/kbe-search.test.mjs:42
- This parity test only checks three text fragments in the extracted production script. It does not verify the fail-closed
incomplete_resultscheck, candidate validation, or the bounded query, so the inline workflow implementation could diverge fromsearch-kbe-issues.cjswhile the test remains green. Add behavior-level parity assertions or execute shared code so this gate covers the security-critical behavior it claims to keep in sync.
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: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
|
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 eval bootstrap issue and additional grader/test gaps must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
.github/workflows/evals/kbe-candidate-reads-grader.mjs:130
- MCP results can carry
{ isError: true }while the event'ssuccessflag remains true; the existing fixer grader explicitly rejects that shape atci-failure-fix.eval.yaml:183-190. This check only looks for[Filtered], so it records an error response as a valid read and can let the eval pass without inspecting the candidate. Require strict success and rejectresult.isErrorhere, with a regression test.
} 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:43
parseCandidatesscans every line and selects the last line that happens to be a JSON array rather than proving that it is the wrapper's stdout. A command such asnode ... query; echo [](or a failed wrapper followed by an empty array) satisfiesisSearchHarnessCall, hides the real candidates, and passes with noissue_readcalls. Parse the exact process output/exit status or reject compound/ambiguous transcripts, and add a regression test.
function parseCandidates(result) {
const text = resultText(result).trim();
const payloads = [text, ...text.split(/\r?\n/).reverse()];
let parsed;
.github/workflows/evals/kbe-candidate-reads-grader.mjs:38
- The repository's existing eval fixtures record GitHub MCP calls as
github-issue_read(evals/test_ci_failure_fix_candidates.py:211-217), and the fixer grader accepts(?:mcp__)?github[-_.]+issue_read. This predicate only recognizes bareissue_reador names ending in.issue_read, so a compliant trajectory using the established name is ignored and the scanner eval fails despite successful reads. Match the namespaced forms and guard non-string tool names.
function isIssueReadCall(event) {
return event.type === "tool_call" &&
(event.data.toolName === "issue_read" || event.data.toolName.endsWith(".issue_read"));
}
.github/workflows/evals/kbe-search.test.mjs:42
- This production-parity test only checks that the inline script contains
execFile, the endpoint, and a token name; it never verifies the newly added fail-closed/metadata invariants (incomplete_results === false, repository/PR filtering, and nonemptyuser.login). Because the production and eval wrappers are duplicated, those paths can drift while this test still passes; execute a shared implementation or assert each invariant in the production path.
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: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
PureWeen
left a comment
There was a problem hiding this comment.
Holistic Review
Motivation: Deterministic REST issue search addresses the semantic-search mismatch in KBE lookup.
Approach: The production search wiring looks coherent, but the eval integration has a reproduced plugin-loading failure.
Summary: Needs Changes at 78f5dae0. Please fix the plugin path below. The already-reported canonical github-issue_read name mismatch also remains; I am not filing it again as a new finding.
Detailed Findings
❌ Error — Grader plugin path is relative to the spec, not the repository
In .github/workflows/ci-eval.yml:324,360, --grader-plugin ./.github/workflows/evals/kbe-candidate-reads-grader.mjs is resolved by pinned Vally 0.14 relative to the eval-spec directory. This produces a duplicated .github/workflows/evals/.github/workflows/evals/ path and fails module import. The exact committed lint invocation reproduced exit 1; changing the argument to ./kbe-candidate-reads-grader.mjs passed strict lint for all three specs.
Use that spec-relative path or an absolute path in both invocations. This corrects the earlier advice in comment 3993276213: the full repository-relative path requested there is not correct for this pinned Vally resolver. Because the eval harness is loaded from the trusted base, this affects the eval variants once this harness change lands; it is not a claim that current PR checks have already exercised it.
The checked-in Node tests and mocked production-handler cases pass, but they do not cover this CLI integration failure. No full 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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
78f5dae to
8557d11
Compare
Summary
issue_readbefore making semantic duplicate decisions.Dependency
This PR depends on dotnet/runtime#133958, which must merge first. The
/ci-evalworkflow restores evaluator inputs frommainbefore checking out the PR head; without the prerequisite files already present onmain, 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
ci-failure-scanwith gh-aw v0.86.2 and actionlint enabled.ci-failure-scan.eval.yaml.ci-eval.yml.Note
This pull request description was generated by GitHub Copilot.