Skip to content

Add data-query category: AL query-generation benchmark - #740

Merged
Onat Buyukakkus (onbuyuka) merged 85 commits into
mainfrom
onbuyuka/data-query-category
Aug 31, 2026
Merged

Add data-query category: AL query-generation benchmark#740
Onat Buyukakkus (onbuyuka) merged 85 commits into
mainfrom
onbuyuka/data-query-category

Conversation

@onbuyuka

@onbuyuka Onat Buyukakkus (onbuyuka) commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

What

Adds a new data-query evaluation category: a benchmark for answering Business Central data
questions through the BC MCP server's Data Query tools
. Given a natural-language question, the agent
must retrieve the real data from a live BC environment using the bc_data_* MCP tools and report
exactly what they return — it cannot answer from general knowledge.

The agent writes two files:

  • answer.json — the result rows that answer the question (one JSON object per row);
  • query.al — the single AL query object it used.

Scoring is execution-based (no LLM judge): build = a parseable answer.json was produced;
resolved (ResolutionRate) = the agent's rows match the gold answer (baked gold_rows, else the
entry's gold_query run live). Rows compare by value — numbers normalized scale-insensitively
(500 == 500.0), Code/No. strings verbatim ("001" != "1"); column names/order ignored;
order-insensitive unless the entry is marked ordered.

Complements the AI Test Toolkit evals in the platform repo: those test the MCP server
end-to-end; this benchmarks models/agents on their ability to use it.

The interesting part: keeping the MCP server the only route to the data

The category is only meaningful if the agent answers through the MCP tools. Making that true was
the bulk of the work — denied one route to the data, the agent kept finding another:

  1. The agent read the leaked connection credentials and hit BC's OData /api directly from a
    shell. Fix: agent_subprocess_env() scrubs every BC_SERVER_* / BC_MCP_* / BC_CONTAINER_NAME
    from the launched agent's environment, and a credential-free localhost MCP gateway
    (src/bcbench/agent/shared/mcp_gateway.py) fronts BC — it injects the auth headers upstream (so the
    agent's MCP config carries no secret, and nothing is recoverable from the process command line) and
    path-restricts to /mcp (so /api is unreachable through it).
  2. The agent then ran docker exec … sqlcmd against the container's SQL database. Mitigated by
    withholding the container name and removing the incentive (a working, easy MCP path); the durable
    fix is network isolation, documented as the end-state.
  3. On the Copilot CLI, custom MCP servers wouldn't load at all — the CLI's MCP registry policy
    fetch returns 403 for the Actions GITHUB_TOKEN, blocking every custom server
    (copilot-cli#4346). Fix: feed a Copilot-licensed
    user PAT via COPILOT_CLI_TOKEN (falls back to github.token). Claude Code is unaffected, so
    the MCP path is validated there first.

Infrastructure

  • BC MCP gateway (agent/shared/mcp_gateway.py) — credential-free, /mcp-only reverse proxy on
    localhost. Warms up BC's tool catalog with retries (a cold insider-29 container can take minutes
    to compose it) and caches tools/list so the agent registers the tools on its first call instead
    of racing a cold, sometimes-dropped server; relays streams faithfully, and strips
    capabilities.experimental from the initialize reply
    — BC advertises x-ms-headerless, which
    otherwise makes Claude's MCP client drop the server.
  • Env scrub (agent/shared/env.py) — removes BC connection vars from the agent subprocess.
  • MCP config (agent/shared/mcp.py) — points the agent at the gateway; independent --bc-mcp /
    --ms-learn-mcp levers.
  • Setup — publishes an AL app (scripts/al/mcp-config-setup/) that provisions the BCBench MCP
    configuration (enables the Data Query tools) and exports the gateway's upstream endpoint. Uses an
    insider BC 29 artifact until the Data Query tools reach a GA artifact (marked TEMPORARY).
  • Agent observability — Claude runs with --output-format=stream-json; tool usage (including
    sub-agent and MCP calls) is parsed from the event stream.
  • Skill + prompt — the bc-al-query-mcp skill and the data-query prompt pin the exact tool names
    (bc_data_find_tables / bc_data_get_table_schema / bc_data_get_table_relations / bc_data_query)
    and their parameters, which stopped the agent from guessing non-existent tool names.

Results

On the Claude path (claude-sonnet-5) the agent genuinely uses the MCP tools — only the real
bc_data_* tools, no name-guessing.

  • A 4-entry test run reaches 4/4 resolved.
  • A full-dataset run
    (run 32968838235) reaches
    9/11 resolved (82%), 11/11 build (100%) — the mechanical "no answer.json" failures are gone
    (every entry warms up, queries via the tools, and writes its answer), and the only two misses are
    genuine data mismatches (a values-differ and a row-count-differ), i.e. real model-accuracy
    signal. With strict exact-match scoring this is on par with / above the AI Test Toolkit's grounded
    judge on the same scenarios.

Getting there needed reliable MCP tool registration: the gateway warms up BC's tool catalog with
retries (a cold insider-29 container can take minutes to compose it) and caches tools/list, so the
agent's client registers the tools on its first call instead of racing a cold server.

Dataset

dataset/dataquery.jsonl — each entry has nl_prompt, gold_query, optional baked gold_rows,
environment_setup_version, and ordered.

How to run (no local containers)

Actions → Evaluation with Claude CodeRun workflow → category data-query, a model, enable
bc-mcp / ms-learn-mcp / skills, test-run = true. The self-hosted GitHub-BCBench
runner provisions the sandbox container, publishes the MCP config app, and stands up the gateway; the
agent answers through the tools; the harness compares its rows to the gold answer.

For the Copilot workflow, set the COPILOT_CLI_TOKEN secret first — otherwise the Copilot CLI's MCP
registry policy fetch fails on the Actions GITHUB_TOKEN and blocks all custom MCP servers
(copilot-cli#4346); the Claude workflow is
unaffected.

Docs

Follow-ups (tracked, not blocking)

  • Set the COPILOT_CLI_TOKEN secret to enable the Copilot CLI path.
  • Bake gold_rows for all entries and revert the temporary build_app timeout headroom.
  • The insider BC 29 artifact hack is temporary, until the Data Query tools are in a GA artifact.
  • Network-isolate the agent (agent-in-a-container) to fully close the docker exec side-door.

Onat Buyukakkus and others added 5 commits July 12, 2026 15:07
Adds a new execution-based `data-query` category that benchmarks models/agents
at generating Business Central AL queries. Given a natural-language data question,
the agent writes a single AL query object to query.al; evaluation compiles and runs
both the generated query and a gold reference query against the container's Contoso
demo data and compares the result sets. No MCP server and no LLM judge.

- types.py: DATA_QUERY -> execution-based (ExecutionBasedEvaluationResult, summary,
  aggregate; resolution_rate/build_rate; ResolutionRate; requires_container; GitHub-BCBench)
- DataQueryEntry: nl_prompt + gold_query + ordered; dataset/dataquery.jsonl (6 tasks)
- DataQueryPipeline + result_sets_match (value-based, order-insensitive; unit-tested)
- operations: wrap_query_as_api (unit-tested) + execute_al_query (wrap as API query,
  publish throwaway app, read OData)
- ExecutionBasedEvaluationResult.create_result for compiled-but-wrong outcomes
- config.yaml: data-query prompt (author query.al); al-query-authoring skill
- Setup-ContainerAndRepository.ps1: skip repo clone for data-query (no repo), just
  provision the sandbox container; a stock Contoso artifact suffices
- Wire data-query into the copilot/claude evaluation workflow category choices + docs

Container round-trip in execute_al_query and the gold AL query bodies need validation
on a runner (no local BC container); pure logic is unit-tested (592 tests pass).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…atch)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…d into container)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21

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.

Looks good. Good stuff.

Comment thread scripts/Setup-ContainerAndRepository.ps1 Outdated
Comment thread src/bcbench/types.py Outdated
Comment thread src/bcbench/types.py Outdated
Onat Buyukakkus and others added 7 commits July 13, 2026 10:55
… (AL0124)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
A gold query failing to compile/run is a harness/dataset problem, not the
agent's, so record it as a non-resolved result with a clear message instead
of letting the uncaught BuildError crash the whole matrix job.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
… sets

The object name is irrelevant to a query's result set (we score by comparing
data), but AL requires it be a valid <=30-char identifier and unique in the
tenant. Two of the first real runs failed only on AL0305 (agent chose a long
descriptive name), so normalize the name in wrap_query_as_api to keep the
benchmark focused on query logic. Also give the generated and gold API queries
distinct EntitySetName/EntityName so both can be published to the same tenant
without colliding on the OData route once a generated query finally compiles.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
… fetch

Root cause of the 0/4 build rate: the agents were writing valid AL (e.g. a
correct Vendor/Purch. Inv. Header query) but the compiler reported base tables
as missing (AL0185, '26.0.0.0 could not be found in the database'). The custom
Compile-AppInBcContainer -UpdateSymbols path did not load Base Application
symbols reliably (intermittent across containers).

Switch execute_al_query to the same Invoke-AppBuildAndPublish helper the passing
categories use (explicit cleared .alpackages symbol folder, GenerateReportLayout
No, ForceSync, dependencyPublishingOption ignore). Also fetch the query rows from
*inside* the container (Invoke-ScriptInBcContainer -> http://localhost:7048/BC/api)
so we no longer depend on host->container name resolution or published ports,
which the runner does not set up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…a fetch

Run 3 showed the compile+publish now works (Base App symbols resolve via the
proven helper), but the in-container OData fetch failed: PowerShell 7 refuses
Invoke-RestMethod -Credential over plain HTTP ('cannot protect plain text
secrets sent over unencrypted connections'). Build the Basic Authorization
header manually instead, which works on both Windows PowerShell 5.1 and
PowerShell 7. Add regression tests asserting the run template uses the proven
build helper, fetches from inside the container, and never passes -Credential
over HTTP.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Run 4 proved the harness works end-to-end (build=2, real gold-vs-generated
result-set comparisons). The remaining resolved=0 was down to prompt ambiguity
and one buggy gold, not the harness:

- Tighten all prompts so a correct interpretation deterministically matches the
  gold: specify the measure and whether it is net of VAT, the source (line vs
  header, posted vs open), inner-join inclusion ('...that has at least one...'),
  and grouping. E.g. the vendor prompt now pins line-level Amount net of VAT
  (a model had reasonably summed header Amount Including VAT -> 5 vs 6 rows).
- Replace 'items on both open orders': its gold expressed a set intersection as
  a join with no aggregate column, so an AL query returns one row per matching
  (sales line x purchase line) pair instead of the distinct item set and cannot
  be scored deterministically (13 vs 12 rows). Swap in a clean aggregate join
  (open sales order count per customer).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Broaden the dataquery benchmark with single-table and clean-join aggregates that
are deterministically scorable via result-set comparison:
- customer-count-by-country (single-table Count)
- outstanding-purchase-value-by-vendor (join + Sum, open POs, net of VAT)
- total-posted-sales-amount-by-customer (2-level join + Sum, net of VAT)
- line-count-per-open-sales-order (single-table Count, child rows per parent)
- total-purchased-quantity-by-item (single-table Sum)

Prompts pin the source table and net-of-VAT measure to avoid the interpretation
ambiguity that made earlier tasks noisy. Field names verified against the W1 Base
App. Gold queries to be confirmed against the container by the evaluation run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Copilot AI review requested due to automatic review settings July 28, 2026 20:33

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

Adds the data-query benchmark for deterministic AL query generation and execution against Business Central demo data.

Changes:

  • Adds 11 query-generation dataset entries and agent guidance.
  • Implements query wrapping, execution, comparison, and result reporting.
  • Integrates container setup, workflows, tests, and documentation.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
dataset/dataquery.jsonl Adds benchmark entries and gold queries.
src/bcbench/evaluate/dataquery.py Implements evaluation and result comparison.
src/bcbench/operations/bc_operations.py Adds query wrapping and OData execution.
src/bcbench/dataset/dataset_entry.py Defines data-query entries.
src/bcbench/dataset/__init__.py Exports the new entry type.
src/bcbench/types.py Registers category runtime behavior.
src/bcbench/results/base.py Adds a general execution-result factory.
src/bcbench/evaluate/__init__.py Exports the pipeline.
src/bcbench/operations/__init__.py Exports query operations.
src/bcbench/commands/evaluate.py Supports mock data-query evaluation.
src/bcbench/agent/shared/config.yaml Adds the agent prompt template.
src/bcbench/agent/shared/instructions/dataquery-bc/skills/al-query-authoring/SKILL.md Adds AL query authoring guidance.
scripts/Setup-ContainerAndRepository.ps1 Creates clone-free workspaces.
scripts/BCBenchUtils.psm1 Resolves the new dataset category.
.github/workflows/copilot-evaluation.yml Enables Copilot runs.
.github/workflows/claude-evaluation.yml Enables Claude runs.
tests/test_dataquery_evaluation.py Tests comparison and wrapping logic.
tests/conftest.py Adds data-query fixtures.
tests/test_type_exhaustiveness.py Covers category type dispatch.
docs/data-query.md Documents the benchmark.
docs/index.md Links the new category.
Comments suppressed due to low confidence (1)

src/bcbench/evaluate/dataquery.py:115

  • execute_al_query also raises BuildTimeoutExpired on a gold-query timeout, and it is not a BuildError. This exception escapes instead of taking the intended harness/container failure path.
        except BuildError as e:
            logger.exception(f"Gold query failed to compile/run for {context.entry.instance_id}")
            self.save_result(
                context,

Comment thread src/bcbench/evaluate/dataquery.py Outdated
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread src/bcbench/evaluate/dataquery.py Outdated
Comment thread src/bcbench/types.py Outdated
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread dataset/dataquery.jsonl Outdated
Scoring integrity:
- result_sets_match: canonicalize numbers with Decimal.normalize() instead of
  rounding through float to 4 decimals, so 1.00001 and 1.00002 are no longer
  scored equal (removes false positives) while 500 == 500.0 still holds.
- OData fetch: follow @odata.nextLink until exhausted so result sets larger than
  one page are not silently truncated (which could score different sets as equal).
- Gold-query failure is now recorded as unscorable (new ExecutionBasedEvaluationResult
  scorable flag) and excluded from resolved/total/build/instance_results, so a
  harness/dataset issue no longer counts against the agent's ResolutionRate.
- Catch BuildTimeoutExpired (not a BuildError) around both generated and gold
  query execution so a timeout is recorded instead of escaping and breaking
  summarization.
- wrap_query_as_api raises BuildError (handled downstream) instead of ValueError
  when the generated output has no query declaration or no object body.

Robustness:
- wrap_query_as_api matches the query keyword and QueryType removal
  case-insensitively and without requiring a leading newline, so cased/compact
  AL (Query 50123, { QueryType = Normal; ... }) no longer breaks ID reassignment
  or produces a duplicate QueryType property.
- execute_al_query uninstalls/unpublishes the throwaway query app before and
  after each run so re-running locally against the same container doesn't fail
  with an object-ID conflict on the fixed 50100/50101 range.

Docs/cleanup:
- SKILL.md: OrderBy is a property (OrderBy = descending(Col);), not a block.
- types.py: drop the stale MCP/seed-app comments; fold DATA_QUERY into the
  existing same-value match arms.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Copilot AI review requested due to automatic review settings July 28, 2026 21:23
@onbuyuka

Copy link
Copy Markdown
Collaborator Author

Review feedback addressed (commit 129c5ef)

Thanks for the thorough review. Summary of what changed:

Scoring integrity

  • Numeric rounding false-positivesresult_sets_match now canonicalizes numbers with Decimal.normalize() (full precision, scale-insensitive) instead of rounding through float to 4 decimals. 1.000011.00002; 500 == 500.0.
  • OData pagination — the fetch now follows @odata.nextLink until exhausted, so large result sets aren't silently truncated to the first page.
  • Gold-failure biasing scores — a gold/harness failure is now recorded as unscorable (new scorable flag) and excluded from resolved/total/build/instance_results, so it no longer counts against the agent's ResolutionRate.
  • Timeout escapingBuildTimeoutExpired is now caught around both generated and gold execution (it isn't a BuildError), so a timeout is recorded instead of breaking summarization.
  • ValueError on malformed outputwrap_query_as_api now raises BuildError (handled downstream) when there's no query declaration or no object body.

Robustness

  • Case sensitivity — the query keyword reassignment and QueryType removal are now case-insensitive and don't require a leading newline, so Query 50123 / { QueryType = Normal; ... } no longer break ID reassignment or create a duplicate QueryType.
  • Object-ID conflict on re-runexecute_al_query now uninstalls/unpublishes the throwaway app before and after each run, so re-running locally against the same container doesn't hit a 50100/50101 conflict.

Docs/cleanup

  • SKILL.md: OrderBy corrected to a property (OrderBy = descending(Col);), not a block.
  • types.py: stale MCP/seed comments removed (folded DATA_QUERY into the existing match arms).
  • PR description: validation scope updated to the current 11 gold queries; the 5 new ones are in runner shakeout now.

Added unit tests for the precision fix, the case-insensitive/malformed wrap_query_as_api paths, and the paging/cleanup wiring. Full suite: 635 pass, ruff + ty clean.

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/bcbench/operations/bc_operations.py:293

  • AL escapes a quote inside a quoted identifier by doubling it ("A ""quoted"" query"), not with a backslash. This regex stops at the first doubled quote, leaves the rest of the original name behind, and turns an otherwise valid query into invalid AL. Match doubled quotes in the quoted-name branch.
    text, replaced = re.subn(
        r'(\bquery\s+)\d+\s+("(?:[^"\\]|\\.)*"|\w+)',
        rf"\g<1>{object_id} {safe_name}",

src/bcbench/evaluate/dataquery.py:31

  • This converts every numeric-looking string to a number, so distinct AL text/code values such as "001" and "1" compare equal (and the earlier None conversion similarly equates null with ""). That can award resolution to a query returning the wrong identifier. Preserve string/null identity and normalize only values known to be numeric, or carry type information into comparison.
    try:
        # Canonical decimal form: scale/trailing-zero-insensitive (500 == 500.0) but full precision
        # preserved, so distinct values like 1.00001 and 1.00002 are NOT collapsed. No float rounding.
        return str(Decimal(text).normalize())
    except (InvalidOperation, ValueError):

Comment thread src/bcbench/results/base.py Outdated
AL query Count columns take no source field: `column(RowCount) { Method = Count; }`,
not `column(RowCount; "No.") { Method = Count; }` (the latter fails AL0353). The
four Count-based golds used the invalid form, and SKILL.md taught it — so the agent
reproduced the mistake and its query failed to compile before the gold was ever
reached, which is why these golds went unvalidated (see PR review comment #15).

- Remove the source field from the Count columns in customer-count-by-country,
  open-sales-order-count-by-customer, opportunity-count-by-status, and
  line-count-per-open-sales-order gold queries.
- SKILL.md: clarify that Count takes no source field, unlike Sum/Average/Min/Max.

Validated by the runner shakeout: the Sum-based new golds (outstanding-purchase-value
-by-vendor, total-purchased-quantity-by-item) already compiled, ran, and resolved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Copilot AI review requested due to automatic review settings July 28, 2026 21:33

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/bcbench/results/base.py:116

  • scorable=False is not propagated to the bc-eval records. category_metrics exports only resolved=False and build=True, and ResolutionRate/BuildRate score those values directly, so the externally reported headline metrics still count a gold-query failure as a resolution failure (and a build success), contrary to the new unscorable semantics. Export scorable and make the downstream evaluators skip these records, or omit unscorable records from the bc-eval export.
    def create_unscorable(cls, context: "EvaluationContext", output: str, error_message: str) -> Self:
        """A harness/dataset failure (not the agent's fault) that must not count toward the resolution rate."""
        return cls(**cls._base_fields(context), output=output, build=True, resolved=False, scorable=False, error_message=error_message)

src/bcbench/operations/bc_operations.py:426

  • The OData JSON is parsed through Python float before _normalize_value sees it, so high-magnitude BC Decimal values can lose precision and distinct results can compare equal (for example, adjacent cent values near BC Decimal's upper range). Parse JSON decimal literals directly as Decimal to preserve the deterministic comparison promised by the matcher.
    rows = json.loads(result_file.read_text(encoding="utf-8-sig") or "[]")

src/bcbench/evaluate/dataquery.py:88

  • The new pipeline's outcome logic is not covered by the added tests: there are no tests that mock execute_al_query and verify match, mismatch, generated build failure, and gold-query unscorable results. These branches define the benchmark's scores, and the current ordering/export issues are examples that helper-only tests do not catch. Add focused pipeline tests like those used for the existing evaluation pipelines.
    def evaluate(self, context: EvaluationContext[DataQueryEntry]) -> None:

Comment thread src/bcbench/evaluate/dataquery.py Outdated
Follow-up to the scorable flag: the local summary already excluded unscorable
results, but write_bceval_results() still exported them, so the uploaded/core
ResolutionRate counted a gold-query harness failure against the agent. Skip
unscorable results in the export path as well.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Copilot AI review requested due to automatic review settings July 28, 2026 21:40
Establish gold validity independent of agent output: run the gold query first, so
a broken gold entry is recorded as unscorable regardless of whether the agent's
query compiled. Previously, if the agent query failed first, a broken dataset
entry was counted against that agent instead of being flagged as a harness issue.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21

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

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/bcbench/operations/bc_operations.py:305

  • The transform is not comment-aware. In a valid query with a preceding comment such as // QueryType = Normal;, this substitution removes the comment occurrence because count=1, leaves the real property, and then injects a second QueryType, causing compilation to fail. Likewise, text.find("{") can select a brace in a leading comment. Locate the declaration/body with comment-aware parsing and remove the actual object-level property rather than the first textual match.
    text = re.sub(r"\bQueryType\s*=\s*\w+\s*;", "", text, count=1, flags=re.IGNORECASE)

    brace_index = text.find("{")
    if brace_index == -1:
        raise BuildError("query-wrap", f"Generated query has no object body ('{{' not found):\n{query_text}")

src/bcbench/agent/shared/instructions/dataquery-bc/skills/al-query-authoring/SKILL.md:27

  • This example is effectively the gold solution for dataquery__outstanding-sales-value-by-customer-1: it uses the same Customer → Sales Line join, Order filter, and Outstanding Amount sum. Any run with this skill enabled receives the answer to a benchmark entry (including the first test-run entry), inflating that experiment's score. Replace it with a valid query pattern that is not represented in the dataset.
            dataitem(SalesLine; "Sales Line")
            {
                DataItemLink = "Sell-to Customer No." = Customer."No.";
                DataItemTableFilter = "Document Type" = const(Order);
                column(OutstandingAmount; "Outstanding Amount") { Method = Sum; }

src/bcbench/results/bceval_export.py:53

  • This scoring-critical skip path has no regression coverage: tests/test_result_writer.py comprehensively exercises write_bceval_results, but no test creates an execution result with scorable=False. Add mixed and all-unscorable cases to verify these records never reach the bc-eval JSONL output.
            # Unscorable results (harness/dataset failures, e.g. a gold query that didn't compile) must
            # not reach the uploaded/core score, or they'd count against the agent's ResolutionRate.
            if isinstance(result, ExecutionBasedEvaluationResult) and not result.scorable:
                logger.info(f"Skipping unscorable result from bceval export: {result.instance_id}")
                continue

Comment thread scripts/Setup-ContainerAndRepository.ps1 Outdated
Diagnosis showed the 'No answer.json' failures were tool-registration
flakiness, not the agent forgetting to write: once the warm-up retry reliably
caches the tool catalog, agents get the data and write answer.json on their own
(the Stop hook fired 0 times in the validated run). Remove the whole Stop-hook
mechanism (hook script, config path, category output-file property,
setup_hooks/_setup_claude_hooks wiring, and tests).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Comment thread .github/workflows/copilot-evaluation.yml Outdated
Comment thread docs/data-query.md Outdated
Comment thread docs/data-query.md Outdated
Comment thread docs/data-query.md Outdated
Comment thread docs/data-query.md
Comment thread src/bcbench/types.py
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread src/bcbench/evaluate/dataquery.py
Comment thread src/bcbench/agent/shared/mcp_gateway.py
Comment thread src/bcbench/dataset/dataset_entry.py Outdated
Onat Buyukakkus and others added 4 commits August 26, 2026 14:29
Two full-run entries produced no answer.json because warm-up retried but BC's
cold tools/list never composed the catalog within the 360s budget. Give BC more
wall-clock time (up to ~5 attempts / 10 min) to catch the slow-cold-start cases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Per review feedback:
- Remove gold_rows baking: gold is now always computed live from gold_query
  (resilient to demo-data changes; live exec measured ~1min). Deletes the
  bake-dataquery-gold command + workflow and the gold_rows field.
- Drop the --ms-learn-mcp dispatch flag on both eval workflows and its
  threading; MS Learn is now toggled purely by presence in config.yaml
  (commented out by default; uncomment on a private branch to A/B it).
- Soften the bc-al-query-mcp skill: MS Learn is 'use if available', not a
  hard dependency; the BC data MCP remains required.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…R review D)

- Split the opaque _QUERY_RUN_TEMPLATE into four clearly-delimited phases
  (cleanup / build+publish / read rows / teardown), each printing a
  timestamped [query-<suffix>] marker via Write-QueryPhase, so a CI run
  shows exactly which phase it reached and where it failed or timed out.
- Document the deliberate fail-loud boundary in DataQueryPipeline.evaluate:
  a broken gold query is a harness/dataset bug and reds the job (via
  _gold_rows), while a missing/malformed answer.json is the agent's own
  task failure, recorded as build=False and surfaced in results rather than
  aborting the matrix job (keeps the benchmark honest, not green-CI-hiding).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
The docs/ folder hosts the GitHub Pages results showcase (leaderboards
driven by site.data), not prose. Rewrite docs/data-query.md to match the
other category pages:
- Brief intro reframed per review: evaluates an agent harness + model (or
  MCP Host) on retrieving data from a live BC environment, comparing a
  no-tooling baseline against the BC MCP Data Query tools experiment.
- Baseline Leaderboard + BC MCP Experiment tables from
  site.data.data-query.aggregate (with the standard 'no results yet' guard).
- Drop the BC-platform-repo reference (audience has no access) and the
  prose scoring/isolation/running sections.
- Update the index.md Data Query row to the live-retrieval framing (was the
  stale offline query-generation description).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Comment thread src/bcbench/commands/dataset.py Fixed
Dropped with the bake-dataquery-gold command that used it (flagged by
github-code-quality).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Comment thread src/bcbench/agent/shared/config.yaml
Comment thread scripts/Setup-ContainerAndRepository.ps1 Outdated
Comment thread .github/workflows/copilot-evaluation.yml Outdated
Comment thread src/bcbench/agent/shared/env.py
Comment thread .gitignore Outdated
Comment thread src/bcbench/agent/shared/mcp_gateway.py
Comment thread src/bcbench/operations/bc_operations.py Outdated
Comment thread src/bcbench/agent/shared/mcp_gateway.py
Comment thread src/bcbench/agent/shared/mcp_gateway.py Outdated
Comment thread src/bcbench/agent/shared/mcp.py Outdated
@haoranpb

Copy link
Copy Markdown
Collaborator

This is getting very close, and I am very excited about it.

We are essentially introducing a way to measure an agent’s ability to retrieve real data from BC, which opens up a very interesting new dimension for BC-Bench.

Onat Buyukakkus and others added 4 commits August 28, 2026 18:55
…meouts, drop --skills

Quick wins:
- Neutralize the data-query prompt: no BC MCP / bc_data_* tool names in the
  base template, so the baseline (no MCP) isn't primed with the treatment.
  Tool specifics live in the skill, which is only present when skills are on.
- Gate the BC MCP-config app publish on the --bc-mcp flag (new -BcMcp switch
  threaded workflow -> action -> Setup-ContainerAndRepository.ps1) instead of
  category == 'data-query', so any category can opt into the BC MCP server.
- Enforce an explicit company in execute_al_query (required arg; drop the
  silent first-company fallback; _gold_rows fails loud if BC_MCP_COMPANY is
  unset) so gold never runs against an arbitrary company.
- Narrow the gateway tools/list typing (walrus, str-checked names).
- Drop the .gitignore '_*/' pattern (clashed with docs/_data, _includes,
  _layouts).

Review decisions:
- Drop the --skills flag from workflows + CLI + agent runners; skills are now
  toggled solely via config.yaml's skills.enabled (fixes the CLI default
  silently overriding config, and the all-skills-at-once variable). Remove
  skills_enabled_override from setup_agent_skills.
- Remove _redact_mcp_config: the container password is short-lived/random and
  already masked in CI via ::add-mask::, and the bcmcp entry is credential-free.
- Give the live gold query its own execute_query timeout (15m) instead of
  bumping the shared build_app budget (restored to 5m).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Per review: the PAT fallback was a temporary testing workaround for the
github.token 403 on /copilot/mcp_registry (github/copilot-cli#4346), which
is being fixed separately. Revert COPILOT_GITHUB_TOKEN to github.token so no
PAT is required to ship; data-query runs use Claude Code in the meantime.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
The merge of #820 (which moved Copilot tool-usage parsing from repo hooks to
stream-json and dropped the REPO_HOOKS env) left this branch still setting
GITHUB_COPILOT_PROMPT_MODE_REPO_HOOKS=true, which #820's security test
(test_copilot_does_not_enable_hooks_memory_or_unrestricted_urls) forbids.
Tool usage is now parsed from stdout stream-json via parse_output, so the
env is vestigial. Remove it, keeping only WORKSPACE_MCP as on main.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21

@haoranpb Sun Haoran (haoranpb) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for working through the comments, I think we are ready now.

Two open comments are for me for follow-up PRs

@onbuyuka
Onat Buyukakkus (onbuyuka) merged commit d767424 into main Aug 31, 2026
28 checks passed
@onbuyuka
Onat Buyukakkus (onbuyuka) deleted the onbuyuka/data-query-category branch August 31, 2026 08:40
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.

6 participants