Skip to content

[aw] Make CI KBE issue search deterministic - #133684

Open
vitek-karas wants to merge 2 commits into
dotnet:mainfrom
vitek-karas:vitek-karas-kbe-search-wrapper
Open

vitek-karas wants to merge 2 commits into
dotnet:mainfrom
vitek-karas:vitek-karas-kbe-search-wrapper

Conversation

@vitek-karas

@vitek-karas vitek-karas commented Sep 11, 2026

Copy link
Copy Markdown
Member

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 dotnet/runtime#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.

@github-actions github-actions Bot added the area-skills Agent Skills label Sep 11, 2026
@azure-pipelines

Copy link
Copy Markdown
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.

@vitek-karas vitek-karas changed the title Make CI KBE issue search deterministic [aw] Make CI KBE issue search deterministic Sep 11, 2026
@vitek-karas
vitek-karas requested a lite review from Copilot September 11, 2026 11:44

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.

🟡 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 0 to 168 hours, while the other agentic workflow lock files still use 0 and 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-calls grader only requires one matching issue_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 inline mcp-scripts block in ci-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 review rule here.
Search for a plain tracker, then read every returned candidate:
  • Files reviewed: 7/7 changed files
  • Comments generated: 4
  • Review effort level: Lite

Comment thread .github/workflows/ci-failure-scan.md Outdated
Comment thread .github/workflows/ci-failure-scan.md
Comment thread .github/workflows/evals/search-kbe-issues.cjs Outdated
Comment thread .github/workflows/evals/README.md Outdated

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.

🟡 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 load kbe-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 into tools.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/logs directory consumed by the parse/upload steps, so MCP-script diagnostics will be lost; generate this path from process.env.RUNNER_TEMP or 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:issue does not enforce the wrapper's scope: a caller can supply a conflicting or negated qualifier such as -repo:dotnet/runtime or is: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 invoking gh, 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, and GITHUB_TOKEN; it never executes or asserts the incomplete_results and 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_results check, candidate validation, or the bounded query, so the inline workflow implementation could diverge from search-kbe-issues.cjs while 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

Comment thread .github/workflows/ci-eval.yml Outdated
Comment thread .github/workflows/evals/kbe-candidate-reads-grader.mjs
Comment thread .github/workflows/ci-failure-scan.lock.yml Outdated
@vitek-karas
vitek-karas marked this pull request as ready for review September 15, 2026 10:06
Copilot AI review requested due to automatic review settings September 15, 2026 10:06
@vitek-karas
vitek-karas requested review from a team and jeffhandley as code owners September 15, 2026 10:06
@azure-pipelines

Copy link
Copy Markdown
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.

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.

🟡 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's success flag remains true; the existing fixer grader explicitly rejects that shape at ci-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 reject result.isError here, 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

  • parseCandidates scans 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 as node ... query; echo [] (or a failed wrapper followed by an empty array) satisfies isSearchHarnessCall, hides the real candidates, and passes with no issue_read calls. 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 bare issue_read or 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 nonempty user.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

Comment thread .github/workflows/ci-eval.yml

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

vitek-karas and others added 2 commits September 15, 2026 18:11
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-skills Agent Skills

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants