fix(gooddata-eval): stop metric-skill simulated user from dropping MAQL clauses - #1718
Open
Tomkess wants to merge 2 commits into
Open
fix(gooddata-eval): stop metric-skill simulated user from dropping MAQL clauses#1718Tomkess wants to merge 2 commits into
Tomkess wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe simulated MAQL response prompt now requires complete, verbatim query preservation and allows 300 tokens. A regression test validates these requirements with a mocked optional OpenAI module. ChangesMAQL prompt generation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gooddata-eval/tests/test_agentic_metric_skill.py`:
- Around line 46-56: Update the assertions in the test covering the generated
prompt to verify that the exact MAQL string from expected_output["maql"] appears
in sent_prompt, preserving the existing prompt checks while ensuring the metric
identifier, label, and filter value are all validated.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57c71e54-018b-427f-b349-92c1767b4d22
📒 Files selected for processing (2)
packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.pypackages/gooddata-eval/tests/test_agentic_metric_skill.py
Tomkess
added a commit
that referenced
this pull request
Aug 6, 2026
…er prompt Addresses CodeRabbit review comment on #1718: the regression test only checked for generic instruction words ("verbatim", "every clause"), not that expected_output["maql"] itself made it into the prompt -- a regression that stripped the metric/label reference or filter value entirely could still pass. Assert the exact MAQL string is present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Fixed in 006ccb5 — added |
6 tasks
Tomkess
added a commit
that referenced
this pull request
Aug 6, 2026
_normalize_maql/_best_maql_match compare an agent's generated MAQL against expected_output.maql via exact string equality after whitespace/wrapper normalization -- but MAQL keywords (SELECT, FOR PREVIOUS, WHERE, BY, ...) are case-insensitive at the query-engine level (confirmed against the MAQL reference), while the comparison itself was fully case-sensitive. Reproduced live in gdc-mic-ai-evaluation, post the #1718 fix: fixture "Create a metric for the prior-year value of Active cards" expects SELECT {metric/active_card_count_-_txn_-_cutcgco} FOR Previous({label/process_date.year}) Agent produced, verbatim: SELECT {metric/active_card_count_-_txn_-_cutcgco} FOR PREVIOUS({label/process_date.year}) Byte-identical except FOR PREVIOUS vs FOR Previous -- scored as a fail. First fix attempt considered and rejected: lowercase everything outside {type/id} braces. That's wrong -- WHERE-clause literal values are ALSO outside braces (e.g. WHERE {label/status} = "Active") and are real, case-sensitive data, not keywords; blindly folding them would create a new false-positive risk (two genuinely different filter values scored as equal). Actual fix: per the MAQL reference, every literal value is quoted and every identifier lives inside {..} -- both are exhaustively structural markers, so protecting text inside either while casefolding everything else needs no keyword list at all (which would risk being incomplete against MAQL's large vocabulary: SELECT, BY, WHERE, HAVING, FOR PREVIOUS/NEXT/EACH, WITHOUT PF, TOP/BOTTOM, WITHIN, RANK family, RUNSUM family, IFNULL, CASE/WHEN, 15+ math functions, ...). Added _casefold_outside_protected(), applied as the final step in _normalize_maql. Tests added: - keyword case-insensitivity on the exact reproduced case (FOR PREVIOUS vs FOR Previous) - identifier case preserved ({metric/Mixed_Case_Id} untouched) - quoted literal case preserved AND still distinguishes real differences (WHERE x = "Active" vs WHERE x = "active" must stay a genuine mismatch -- this is the test that would have caught the rejected first draft) Updated the one existing test whose expected value assumed no case normalization ever happens (SELECT -> select). Full gooddata-eval suite: 274 passed, 9 pre-existing unrelated failures (missing openai extra in this test env; two unrelated test files) -- identical count to before this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g MAQL clauses
agentic_metric_skill's simulated-user reply (generate_simulated_response) is
what keeps a multi-turn metric-creation conversation going after the agent
asks a clarifying question -- it prompts an LLM to answer as the user, using
the fixture's expected_output.maql as its only source of truth.
The prompt told it to "reply briefly" with no instruction to preserve the
MAQL's structure. In practice it would silently drop a WHERE/filter clause,
or paraphrase a label id, whenever the agent's question didn't happen to ask
about that part directly -- so a well-behaved agent, faithfully following
the (already-wrong) simulated answer, still failed the eval.
Reproduced live twice against a real gdc-mic-ai-evaluation fixture
("Create a metric for total ecommerce spend", expects
SELECT {metric/spend_amount_-_cutcgco} WHERE {label/ecommerce_indicator_code}
= "1"):
1. Simulated reply dropped "_code" off ecommerce_indicator_code, anchoring
the agent on a sibling attribute that doesn't have that filter.
2. Simulated reply picked one of 3 metric options the agent offered and
said "please proceed with that" -- never mentioning the WHERE clause
that expected_output required, even though it had it in hand.
Confirmed via a 5x-repeated A/B test that this is a prompt problem, not a
model-capability one: swapping gpt-4o-mini for gpt-4o under the OLD prompt
did not fix it (still dropped the clause); the NEW prompt fixes it on the
ORIGINAL gpt-4o-mini (1/5 -> 5/5 runs preserving the exact filter).
Fix: instruct the simulating LLM to (a) ensure every clause of the expected
MAQL is eventually satisfied even if the agent's question didn't ask about
it, (b) quote field/label identifiers verbatim rather than paraphrase them,
and (c) proactively add a filter the agent's own offered options omitted.
Also drop "reply briefly" and raise max_tokens 150->300, since brevity was
part of what squeezed the filter clause out. This brings metric_skill's
simulated-user prompt in line with alert_skill's generate_simulated_alert_response,
which already passes structured facts + explicit "proactively tell the agent
X" instructions rather than one freely-paraphrased string -- not a new
pattern for this codebase.
Added a regression test asserting the sent prompt preserves clause-fidelity
language and the raised max_tokens. Full gooddata-eval suite: 272 passed
(9 pre-existing unrelated failures, confirmed identical on clean master
before this change -- missing openai extra in test env, and two unrelated
test files).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…er prompt Addresses CodeRabbit review comment on #1718: the regression test only checked for generic instruction words ("verbatim", "every clause"), not that expected_output["maql"] itself made it into the prompt -- a regression that stripped the metric/label reference or filter value entirely could still pass. Assert the exact MAQL string is present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tomkess
force-pushed
the
fix/metric-skill-simulated-user-prompt-fidelity
branch
from
August 19, 2026 14:39
006ccb5 to
ecf8718
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1718 +/- ##
==========================================
+ Coverage 79.50% 79.53% +0.03%
==========================================
Files 272 272
Lines 19019 19019
==========================================
+ Hits 15121 15127 +6
+ Misses 3898 3892 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agentic_metric_skill's simulated-user step (generate_simulated_responseinmetric_skill.py) is what keeps a multi-turn metric-creation conversationgoing after the agent asks a clarifying question — it prompts an LLM to answer
as the user, using the fixture's
expected_output.maqlas its only source oftruth.
The prompt told it to "reply briefly," with no instruction to preserve the
MAQL's structure. In practice this let it silently drop a
WHERE/filterclause, or paraphrase a label id, whenever the agent's question didn't happen
to ask about that specific part — so a well-behaved agent, faithfully
following the (already-wrong) simulated answer, still failed the eval through
no fault of its own.
Reproduced live (twice), against a real
gdc-mic-ai-evaluationfixtureQuestion: "Create a metric for total ecommerce spend"
Expected:
SELECT {metric/spend_amount_-_cutcgco} WHERE {label/ecommerce_indicator_code} = "1"_codeoffecommerce_indicator_code, anchoringthe agent on a sibling attribute that doesn't carry that filter — cascaded
into a follow-up the agent's own clarification-detection didn't recognize
as needing a reply (separate, related issue, not fixed in this PR — see
_is_asking_clarification's narrow keyword list).said "please proceed with that" — never mentioning the
WHEREclauseexpected_outputrequired, even though it had the full MAQL in hand.Confirmed: prompt problem, not a model-capability problem
Ran the same
agent_message/expected_maqlpair throughclient.chat.completions.createdirectly, 2×2, isolating model from prompt:Then repeated A and C 5× each to rule out luck: old prompt 1/5 preserved
the exact field (and was unstable even in which base metric it picked — 2/5
picked a different one entirely); new prompt 5/5.
The fix
is eventually satisfied even if the agent's question didn't ask about it,
(b) quote field/label identifiers verbatim rather than paraphrase, (c)
proactively add a filter the agent's own offered options omitted.
max_tokens150→300 — brevity was part of whatsqueezed the filter clause out.
metric_skill.py's simulated-user prompt in line withalert_skill.py'sgenerate_simulated_alert_response, which already passesstructured facts + explicit "proactively tell the agent X" instructions
instead of one freely-paraphrased string — not a new pattern for this
codebase, just extending an existing one to
metric_skill.What this does NOT fix (tracked separately, not in scope here)
_is_asking_clarification's narrow keyword match ("?","could you","please provide","clarif") can miss a legitimate open-ended fork/offerfrom the agent and end the conversation early. Surfaced during the same
investigation, but a separate fix with its own risk profile — kept out of this
PR to keep the diff reviewable.
Test plan
test_generate_simulated_response_prompt_preserves_maql_fidelity—asserts the sent prompt preserves clause-fidelity language and the
raised
max_tokens.gooddata-evalsuite: 272 passed. 9 pre-existing failuresconfirmed identical on clean
masterbefore this change (missingopenaiextra in this test env fortest_llm_judge.py, unrelatedfailures in
test_runner.py/test_summary_evaluator.py) — this changeintroduces zero new failures.
ruff check/ruff format --checkclean on both changed files.the fix resolves it, on both the affected files' logic and via direct
OpenAI API A/B testing (5× per condition).
Summary by CodeRabbit
Bug Fixes
Tests