Add data-query category: AL query-generation benchmark - #740
Add data-query category: AL query-generation benchmark#740Onat Buyukakkus (onbuyuka) wants to merge 77 commits into
Conversation
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
Magnus Hartvig Grønbech (Groenbech96)
left a comment
There was a problem hiding this comment.
Looks good. Good stuff.
… (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
There was a problem hiding this comment.
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_queryalso raisesBuildTimeoutExpiredon a gold-query timeout, and it is not aBuildError. 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,
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
Review feedback addressed (commit 129c5ef)Thanks for the thorough review. Summary of what changed: Scoring integrity
Robustness
Docs/cleanup
Added unit tests for the precision fix, the case-insensitive/malformed |
There was a problem hiding this comment.
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 earlierNoneconversion 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):
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
There was a problem hiding this comment.
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=Falseis not propagated to the bc-eval records.category_metricsexports onlyresolved=Falseandbuild=True, andResolutionRate/BuildRatescore 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. Exportscorableand 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
floatbefore_normalize_valuesees 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 asDecimalto 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_queryand 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:
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
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
There was a problem hiding this comment.
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 becausecount=1, leaves the real property, and then injects a secondQueryType, 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.pycomprehensively exerciseswrite_bceval_results, but no test creates an execution result withscorable=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
…nect Keep BC's exact SSE bytes for a POST reply (some MCP clients require the event-stream framing) but stop as soon as the JSON-RPC response event arrives, so the held-open stream doesn't stall the client. Also treat a client disconnecting mid-stream as normal instead of logging it as an upstream failure, and add de-stream timing/response-seen logging to diagnose the remaining Claude bcmcp handshake failure in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…dshake Claude receives a valid initialize response (response_seen=True) then aborts before notifications/initialized, only against real BC (not the local mock). Log BC's initialize result content and the exact headers the gateway forwards on the relayed SSE, to pin down what real BC returns that Claude rejects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…/list BC's initialize result advertises experimental x-ms-headerless and BC keeps the initialize SSE stream open as the client's event channel. Closing/collapsing that stream made Claude abort right after initialize (never sending notifications/initialized). Relay every response byte-for-byte and keep streams open exactly as BC does, so streamable-HTTP clients see the real transport. The only short-circuit is the tools/list warm-up cache (now framed as a single-event SSE to match BC), which avoids BC's slow/dropping per-session tool-catalog composition. Removes the POST SSE de-stream path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Bisected against a byte-exact replica of BC's initialize response: the sole
cause of Claude marking bcmcp 'failed' is capabilities.experimental =
{x-ms-headerless: true}. The gateway now rewrites just that first initialize
result event to drop capabilities.experimental, then keeps relaying faithfully
(stream held open like BC). BC still works over the standard header-based
session the warm-up probe uses, so dropping the advertisement is safe.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
The first genuinely-connected Claude run resolved a data question with the BC MCP tools, but other agents wasted turns guessing non-existent tool names (find_tables, search_tables, list_tables, query, run_query, get_schema). Add an explicit tool contract to the skill (the four exact names bc_data_find_tables / bc_data_get_table_schema / bc_data_get_table_relations / bc_data_query with their real parameters searchText/searchMode, tableId/nameContains, queryText/returnData) and a no-guessing rule, and name the exact tools in the data-query prompt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Remove the debugging instrumentation added while diagnosing the BC MCP path, keeping only the functional behavior: - mcp_gateway: drop verbose per-request/init-result logging, the raw-response diagnostic strings, and the redundant direct-vs-gateway dual probe; a single warm_up() now primes BC and caches tools/list. - claude: drop the transcript-file dump and session-init logging (stream-json tool_usage parsing stays). - workflows: revert the *.log artifact upload to jsonl-only and remove the NST log-capture step. - delete scripts/Capture-NstLogs.ps1 and tools/run_gateway_local.py. The BC insider-29 artifact hack and its build_app headroom are left in place. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…ategory # Conflicts: # .github/workflows/claude-evaluation.yml # .github/workflows/copilot-evaluation.yml # scripts/BCBenchUtils.psm1 # src/bcbench/dataset/__init__.py # src/bcbench/evaluate/__init__.py # src/bcbench/types.py # tests/conftest.py # tests/test_type_exhaustiveness.py
- Rewrite docs/data-query.md for the MCP-based design (agent answers via the bc_data_* tools, writes answer.json, scored against gold rows). - Add three findings reports under docs/data-query/: custom MCP servers not loading in GitHub Copilot CLI in Actions; the agent bypassing MCP via BC's API (and the credential-free gateway that closes it); the agent bypassing via direct SQL and the isolation end-state. - Refresh the env-scrub comment to reflect the gateway injecting auth. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…ting) Validate that upstream header keys/values contain no CR/LF before relaying them to the client, closing the CodeQL HTTP response-splitting alerts on the gateway relay paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
- Remove the three docs/data-query/ write-ups (kept as local notes, not PR content) and drop their links from docs/data-query.md. - ruff format the gateway (a long call the pre-commit ruff-format flagged). - Fix ty None-safety in the gateway tests (typed recording server, assert the gateway/base_url/config are non-None, guard urlsplit hostname) so 'pre-commit run ty check .' passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
- bc_operations.execute_al_query: pin the gold query to a specific company (passed through and matched by name) instead of the arbitrary first company, and type suffix as Literal['generated', 'gold']. Callers pass BC_MCP_COMPANY so gold runs against the same company the agent queried via MCP. - summary.from_results: drop the no-op 'total': total re-assignment. - Tests: render helper passes the new company key; add a company-pinning assertion. 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
…fore merge The BC 29 insider-feed artifact hack is intentionally part of this PR (the Data Query tools only exist in BC 29, which is not GA yet), so the 'remove before merge' note was misleading. Retarget both markers to 'remove once BC 29 is GA'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
The dominant data-query failure was 'No answer.json produced' - the agent
queried correctly but finished with a chat answer, skipping the file the
evaluation reads. Add a Claude Stop hook that blocks completion while the
required output file is missing (bounded retries), so a correct querier is no
longer scored zero for not writing the file.
- require-answer-file.ps1: Stop hook returning {decision:block} until the file
exists, capped by a retry counter.
- setup_hooks gains required_output_file; the Claude runner passes
EvaluationCategory.required_agent_output_file (answer.json for data-query).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
…top hook The Stop-hook validation run was mixed (2 fixed, 2 regressed with ~3x more bc_data calls). Re-add the transcript dump + *.log artifact upload to see whether the Stop hook fires and whether it drives re-querying instead of a file write. To be reverted once diagnosed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
Diagnosis of the Stop-hook run: the 'No answer.json' failures were not the agent forgetting to write - the bcmcp tools never registered in those sessions. The gateway warm-up (which caches tools/list) timed out for every entry, so the cache stayed empty and each agent session raced BC's cold, often-dropped first tools/list; when it lost, the agent hunted for tools for minutes and gave up. Warm-up now retries the initialize->tools/list handshake until BC returns the catalog (bounded per attempt, total budget 360s), so the cache is reliably populated before the agent starts and its tools/list is served instantly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
The warm-up retry fix is validated: with the tool catalog reliably cached, all entries now register the bc_data tools and write answer.json on their own (the Stop hook did not even need to fire). Remove the temporary transcript dump and *.log artifact upload added for diagnosis. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: db34a2a0-7035-4361-b911-becb72f86e21
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
| Because the category measures whether the agent can answer *through the MCP tools*, the harness goes to | ||
| some length to make sure it cannot reach the data any other way — denied one route, an agent will look | ||
| for another (reading leaked connection credentials to hit BC's OData `/api`, or `docker exec … sqlcmd` | ||
| against the container's database). |
There was a problem hiding this comment.
[documentation] Isn't the rational behind blocking BC's OData /api is because, in Production (SaaS), a coding agent won't be able to query data this way?
If a coding agent is able to query API in production, it be a very interesting scenario to compare the result between using AL query object VS direct API calls.
Only block API endpoints if in production (SaaS), the API path is genuine unavailable.
There was a problem hiding this comment.
Good question — the rationale is exactly the prod path. In prod SaaS /api is not disabled, but it requires an OAuth bearer token (app registration or a user token); a coding agent will not have that configured, which is precisely why the MCP server (backed by APIs) exists. So blocking /api in eval approximates the realistic prod scenario where the agent''s only practical route to the data is the MCP tools.
You are right that if an agent did hold API creds in prod, AL-query-via-MCP vs. direct-API would be a genuinely interesting comparison — a nice future experiment, but it needs the agent to carry OAuth creds, which is out of scope here. (See the mcp_gateway.py thread for how the /api block is enforced and the drift analysis.) Leaving open in case you want to shape the wording further.
There was a problem hiding this comment.
[critical] Is there any other way that we could address this? e.g. disable API in the docker BC env?
Adding the MCP gateway introduce a drift between Production behavior and Evaluation, we would ike the two to be as close as possbile, so the performance gain in evaluation will most likely happen in Production.
There was a problem hiding this comment.
Good push — I dug into whether we can drop the gateway or disable /api in the container instead. Summary: the gateway can be trimmed but not eliminated today, and the drift is confined to transport/auth plumbing, not the behavior we measure.
Why not just disable /api in the BC container? The harness computes the gold rows by running the reference AL query over /api from inside the container (Invoke-ScriptInBcContainer -> http://localhost:7048/BC/api). BC on-prem serves /api and /mcp on the same port (7048) behind the same Basic auth, and there is no BC setting to disable only /api at the path level — ApiServicesEnabled=false kills the whole API surface the gold path depends on. A network/firewall block cannot help either, since it cannot distinguish /api from /mcp on one port. So enforcing "agent can reach /mcp but not /api" needs an L7 filter in front -> the gateway.
Is the drift meaningful? The MCP protocol path the agent exercises — tools/list, bc_data_find_tables / get_table_schema / get_table_relations / bc_data_query — is identical to prod. The gateway differs only in transport/auth plumbing: it injects the Basic auth upstream so the agent''s MCP config is credential-free (which also closes the credential-leak side-door). It does not change tool inputs/outputs, so the eval->prod transfer risk on the thing we measure (can the agent use the Data Query tools to get correct data) is low.
Prod realism of the /api block: in prod SaaS /api requires an OAuth bearer token (app registration / user token) a coding agent will not have; the API-backed MCP server is the realistic path, so blocking /api in eval approximates it. (Cross-linked with the docs /api thread.)
Maintainability: the gateway is a single module. Its non-isolation functions each solve a concrete BC-on-insider-29 issue rather than being gratuitous: credential injection (keeps creds out of the agent), the x-ms-headerless strip (a real Claude MCP-client incompatibility with BC''s initialize capabilities.experimental), and the tools/list warm-up cache (BC''s cold per-session catalog composition is slow and drops the connection). Those would still be needed even if isolation were solved another way.
Net: I would keep it, and revisit if BC ever (a) exposes /mcp on a separate port/instance from /api, or (b) we compute gold without /api. Open to a cleaner isolation primitive if you see one.
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
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
What
Adds a new
data-queryevaluation category: a benchmark for answering Business Central dataquestions 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 reportexactly 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 ALqueryobject it used.Scoring is execution-based (no LLM judge):
build= a parseableanswer.jsonwas produced;resolved(ResolutionRate) = the agent's rows match the gold answer (bakedgold_rows, else theentry's
gold_queryrun 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.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:
/apidirectly from ashell. Fix:
agent_subprocess_env()scrubs everyBC_SERVER_*/BC_MCP_*/BC_CONTAINER_NAMEfrom 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 theagent's MCP config carries no secret, and nothing is recoverable from the process command line) and
path-restricts to
/mcp(so/apiis unreachable through it).docker exec … sqlcmdagainst the container's SQL database. Mitigated bywithholding the container name and removing the incentive (a working, easy MCP path); the durable
fix is network isolation, documented as the end-state.
fetch returns
403for the ActionsGITHUB_TOKEN, blocking every custom server(copilot-cli#4346). Fix: feed a Copilot-licensed
user PAT via
COPILOT_CLI_TOKEN(falls back togithub.token). Claude Code is unaffected, sothe MCP path is validated there first.
Infrastructure
agent/shared/mcp_gateway.py) — credential-free,/mcp-only reverse proxy onlocalhost. Warms up BC's tool catalog with retries (a cold insider-29 container can take minutes
to compose it) and caches
tools/listso the agent registers the tools on its first call insteadof racing a cold, sometimes-dropped server; relays streams faithfully, and strips
capabilities.experimentalfrom the initialize reply — BC advertisesx-ms-headerless, whichotherwise makes Claude's MCP client drop the server.
agent/shared/env.py) — removes BC connection vars from the agent subprocess.agent/shared/mcp.py) — points the agent at the gateway; independent--bc-mcp/--ms-learn-mcplevers.scripts/al/mcp-config-setup/) that provisions theBCBenchMCPconfiguration (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).--output-format=stream-json; tool usage (includingsub-agent and MCP calls) is parsed from the event stream.
bc-al-query-mcpskill 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 realbc_data_*tools, no name-guessing.(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 theagent's client registers the tools on its first call instead of racing a cold server.
Dataset
dataset/dataquery.jsonl— each entry hasnl_prompt,gold_query, optional bakedgold_rows,environment_setup_version, andordered.How to run (no local containers)
Actions → Evaluation with Claude Code → Run workflow → category
data-query, a model, enablebc-mcp / ms-learn-mcp / skills, test-run =
true. The self-hostedGitHub-BCBenchrunner 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_TOKENsecret first — otherwise the Copilot CLI's MCPregistry policy fetch fails on the Actions
GITHUB_TOKENand blocks all custom MCP servers(copilot-cli#4346); the Claude workflow is
unaffected.
Docs
docs/data-query.md— category overview (rewritten for the MCP design).Follow-ups (tracked, not blocking)
COPILOT_CLI_TOKENsecret to enable the Copilot CLI path.gold_rowsfor all entries and revert the temporarybuild_apptimeout headroom.docker execside-door.