Skip to content

feat(gooddata-eval): add KDA-skill agentic evaluator - #1706

Open
FrankHuynh wants to merge 1 commit into
masterfrom
QA-28800-kda-skill
Open

feat(gooddata-eval): add KDA-skill agentic evaluator#1706
FrankHuynh wants to merge 1 commit into
masterfrom
QA-28800-kda-skill

Conversation

@FrankHuynh

@FrankHuynh FrankHuynh commented Aug 4, 2026

Copy link
Copy Markdown

What

Adds kda_skill.py to gooddata-eval, evaluating the chatbot's create_key_driver_analysis / execute_key_driver_analysis tool calls against the agent_kda_skill Langfuse dataset.

Related: QA-28800 — Build E2E LLM test for KDA skill.

Scope

Current scope is completion, not field correctness — decided with the team mid-implementation (originally the design asserted per-field correctness; narrowed to a performance/completion focus for this first pass):

strict_pass = kda_triggered AND executed AND success AND turn_completed

Per-field checks (Measure / Date Attribute / Analyzed Period / Reference Period / Filters / Summary within tolerance) are still computed and logged to Langfuse as informational scores (all kda_-prefixed, e.g. kda_measure_correct, so none can ever collide with another skill's own score of the same shape) — so a follow-up correctness ticket can promote them to strict_pass without redoing the extraction logic — but they do not gate pass/fail here.

Latency / performance reporting

This PR logs the whole-turn latency for each run, but does not bucket it into pass/failed/error or own a report threshold — that classification lives entirely in gdc-nas's combo_report.py. An earlier revision exported a threshold/classifier here for combo_report.py to import, but that cross-repo import never actually resolves in the report-generation CI job (it runs via a bare uv run --with pyyaml --with requests ..., with no visibility into this package regardless of what's pinned) — so gdc-nas's PR moves that logic in-repo instead, self-contained.

One caveat still worth flagging: the latency logged here is the whole conversational turn (LLM planning/orchestration + every tool call), not isolated KDA-backend execution time — a slow turn means "the turn that happened to involve KDA was slow," not conclusively "the KDA backend is slow." Isolating KDA's own execution time (from the observation bracketing the tool call) is implemented on the gdc-nas side, not here.

Disambiguation safety net

KDA cases are designed to resolve in one turn, but if the agent asks a clarifying question instead of triggering KDA directly (a metric-title collision, or a choice between the metric-id and the mathematically equivalent ad-hoc fact+SUM form of the same measure), a simulated-user reply — mirroring alert_skill.py / metric_skill.py's existing pattern (gpt-4o-mini) — picks any acceptable candidate and continues, bounded to 2 turns. This keeps a disambiguation turn from blocking the actual thing being measured: whether KDA itself triggers and completes.

The "is this a clarifying question?" check (previously copy-pasted into metric_skill.py and a drifted third copy in conversation.py) is now a single shared _clarification.py, tightened to require the message actually end on a question rather than just contain "?" anywhere.

Cross-cutting fix: value_score no longer treats unknown latency/cost as the best or worst outcome

log_quality_and_value_scores (used by every agentic skill — alert, metric, viz, conversation, search, guardrail, general_question, not just KDA) substituted speed=0.0/cost_factor=0.0 whenever latency or cost couldn't be resolved yet, silently pulling value_score toward the worst possible outcome for a run that may well have been fast/cheap. It now drops that weighted term entirely and renormalizes over whichever components do have a real value, so value_score only ever reflects signals actually measured for that run. Only changes behavior when latency/cost is None; the common case (all three known) is bit-for-bit identical to before.

Verification

No local Tiger instance available, so verified two ways:

  • Offline unit checks against synthetic and real captured data (ruff check, ruff format --check, ty check, py_compile all clean; _evaluate_run exercised directly with real SSE payloads captured from a 30-run manual stability test against the target workspace — including the exact "KDA computed correctly but the chat turn died silently" case, which correctly fails strict_pass via turn_completed).
  • Defensive-parsing guards (_to_number, isinstance checks before treating a value as a dict) added to match alert_skill.py's existing risk tolerance for malformed tool-call payloads — not a new risk, just consistent handling.
  • Package-level import (from gooddata_eval.core.agentic import evaluate_agentic_kda_skill, ...) verified to resolve with no circular-import issues after registering the new module in __init__.py.

Not included in this PR

  • Version bump / release — will follow in a separate, explicitly-confirmed step once this is reviewed (the version is monorepo-shared across all 9 published packages, so bumping is a deliberate, separate action).
  • The gdc-nas side (shim, tavern test, fixtures pulled from Langfuse, cron wiring, the daily report's KDA table) — tracked under QA-28800, companion PR in gdc-nas.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added support for evaluating agentic KDA skills through realistic conversational workflows.
    • Evaluations can handle clarification questions, validate results, and assess triggering, execution, completion, and informational accuracy.
    • Added repeated-run evaluation with pass-rate summaries and best-result reporting.
    • Added optional tracing and scoring integrations for evaluation observability.
    • Exposed evaluation results, summaries, and detailed assertion feedback through the public package interface.
    • Improved trace selection for more reliable evaluation reporting.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds an agentic KDA skill evaluator. It validates KDA results, supports clarification turns and repeated runs, integrates optional Langfuse tracing and scoring, and exposes the new API through the agentic package.

Changes

Agentic KDA evaluation

Layer / File(s) Summary
KDA evaluation contracts and correctness
packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py
Adds input normalization, clarification detection, tool-call extraction, result types, process gates, and informational correctness checks.
KDA execution and aggregation
packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py
Runs KDA conversations with bounded clarification retries, manages conversation cleanup, and calculates pass-at-k and pass-power-k results.
Langfuse trace discovery and scoring
packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.py, packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py
Retrieves observations, selects KDA traces, and logs optional evaluation scores.
KDA public package exports
packages/gooddata-eval/src/gooddata_eval/core/agentic/__init__.py
Exports KDA result types, assertion errors, evaluation helpers, and run helpers.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Evaluator
  participant GoodDataAPI
  participant OpenAI
  participant Langfuse
  Evaluator->>GoodDataAPI: Create conversation and send question
  GoodDataAPI-->>Evaluator: Return agent messages and tool-call events
  Evaluator->>OpenAI: Generate simulated clarification reply
  OpenAI-->>Evaluator: Return clarification response
  Evaluator->>GoodDataAPI: Send reply and collect execution result
  Evaluator->>Langfuse: Discover traces and log evaluation scores
Loading

Suggested reviewers: hkad98, lupko, pcerny

Poem

A rabbit checks each KDA run,
Through clarifying turns it hops.
It counts the passes, one by one,
Then scores the traces when it stops.
New exports bloom in the package tree.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding an agentic KDA skill evaluator to gooddata-eval.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.68932% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.85%. Comparing base (d1ab1ad) to head (a1beca9).

Files with missing lines Patch % Lines
...a-eval/src/gooddata_eval/core/agentic/kda_skill.py 92.81% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1706      +/-   ##
==========================================
+ Coverage   78.59%   78.85%   +0.25%     
==========================================
  Files         271      273       +2     
  Lines       18772    18962     +190     
==========================================
+ Hits        14754    14952     +198     
+ Misses       4018     4010       -8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (4)
packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py (4)

55-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Normalize expected the same way as actual.

Line 201 passes expected.get("Filters", []). If a dataset item contains "Filters": null, expected is None. json.dumps(None) produces "null", and actual is normalized to [], so filters_correct becomes False for a semantically empty expectation. A follow-up ticket plans to promote this field into strict_pass, so fix the baseline now.

♻️ Proposed normalization
-def _filters_match(actual: object, expected: list) -> bool:
+def _filters_match(actual: object, expected: list | None) -> bool:
     actual = actual or []
+    expected = expected or []
     try:
         return json.dumps(actual, sort_keys=True) == json.dumps(expected, sort_keys=True)
     except TypeError:
         return False
🤖 Prompt for 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.

In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around
lines 55 - 60, Update _filters_match to normalize expected the same way as
actual before comparing serialized values, so None is treated as an empty filter
list and expected.get("Filters", []) remains semantically consistent with
missing filters. Preserve the existing TypeError handling and comparison
behavior for non-null values.

96-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Filter non-dict candidates before calling .get.

measure_candidates comes from the dataset expected_output. If the list contains a non-dict element, line 98 raises AttributeError. _measure_matches already guards this shape at line 52 with isinstance(c, dict). Apply the same guard here.

🛡️ Proposed guard
-    candidates = measure_candidates if isinstance(measure_candidates, list) else [measure_candidates or {}]
+    raw = measure_candidates if isinstance(measure_candidates, list) else [measure_candidates or {}]
+    candidates = [c for c in raw if isinstance(c, dict)]
     candidate_desc = "; or ".join(
🤖 Prompt for 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.

In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around
lines 96 - 100, Update the candidate construction used by candidate_desc to
filter list elements through isinstance(c, dict), matching the shape guard in
_measure_matches. Ensure only dictionary candidates reach the generator
expression and its .get calls, while preserving the existing fallback behavior
for non-list measure_candidates.

107-111: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Set timeout=30.0 on the OpenAI call.

This prevents the evaluation path from waiting for the client's long default timeout.

🤖 Prompt for 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.

In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around
lines 107 - 111, Update the OpenAI request in the client.chat.completions.create
call to pass timeout=30.0, ensuring the evaluation path uses the explicit
30-second timeout while preserving the existing model, messages, and max_tokens
arguments.

189-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Narrow the optionals directly instead of through intermediate boolean guards.

Runtime behavior is correct because and short-circuits. Type checkers, however, do not narrow dict | None through intermediate boolean variables. Type narrowing occurs only through direct conditions in and and if statements. Direct narrowing improves clarity and prevents type-checker warnings when static analysis is enabled.

♻️ Proposed narrowing
-    success = executed and execute_result.get("success") is True
+    success = execute_result is not None and execute_result.get("success") is True
 
     # Informational only (see KdaEvaluation docstring) -- still computed so a follow-up
     # ticket can promote these to strict_pass without redoing the extraction logic.
-    measure_correct = kda_triggered and _measure_matches(create_args.get("measure"), expected.get("Measure"))
-    date_attribute_correct = kda_triggered and create_args.get("date_attribute_id") == expected.get("Date Attribute")
-    analyzed_period_correct = kda_triggered and create_args.get("analyzed_period") == expected.get("Analyzed Period")
-    reference_period_correct = kda_triggered and create_args.get("reference_period") == expected.get("Reference Period")
-    filters_correct = kda_triggered and _filters_match(create_args.get("filters"), expected.get("Filters", []))
+    args = create_args or {}
+    measure_correct = kda_triggered and _measure_matches(args.get("measure"), expected.get("Measure"))
+    date_attribute_correct = kda_triggered and args.get("date_attribute_id") == expected.get("Date Attribute")
+    analyzed_period_correct = kda_triggered and args.get("analyzed_period") == expected.get("Analyzed Period")
+    reference_period_correct = kda_triggered and args.get("reference_period") == expected.get("Reference Period")
+    filters_correct = kda_triggered and _filters_match(args.get("filters"), expected.get("Filters", []))
🤖 Prompt for 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.

In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py` around
lines 189 - 201, Update the correctness calculations in the evaluation flow
around kda_triggered and create_args so each optional create_args access is
guarded by a direct create_args is not None condition in the same and
expression. Remove reliance on the intermediate kda_triggered boolean for type
narrowing, while preserving kda_triggered for reporting and the existing
matching logic.
🤖 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/src/gooddata_eval/core/agentic/kda_skill.py`:
- Around line 266-269: Contain failures from generate_simulated_kda_response
within the clarification loop in _run_once: catch its dependency, configuration,
and API exceptions, log them through a module-level _log logger, and terminate
only the current run while preserving already-completed runs and allowing
evaluate_agentic_kda_skill to continue to Langfuse logging and final assertion.
Add the requested logging import and module-level logger.

---

Nitpick comments:
In `@packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py`:
- Around line 55-60: Update _filters_match to normalize expected the same way as
actual before comparing serialized values, so None is treated as an empty filter
list and expected.get("Filters", []) remains semantically consistent with
missing filters. Preserve the existing TypeError handling and comparison
behavior for non-null values.
- Around line 96-100: Update the candidate construction used by candidate_desc
to filter list elements through isinstance(c, dict), matching the shape guard in
_measure_matches. Ensure only dictionary candidates reach the generator
expression and its .get calls, while preserving the existing fallback behavior
for non-list measure_candidates.
- Around line 107-111: Update the OpenAI request in the
client.chat.completions.create call to pass timeout=30.0, ensuring the
evaluation path uses the explicit 30-second timeout while preserving the
existing model, messages, and max_tokens arguments.
- Around line 189-201: Update the correctness calculations in the evaluation
flow around kda_triggered and create_args so each optional create_args access is
guarded by a direct create_args is not None condition in the same and
expression. Remove reliance on the intermediate kda_triggered boolean for type
narrowing, while preserving kda_triggered for reporting and the existing
matching logic.
🪄 Autofix (Beta)

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

Run ID: a4c9e8b3-9e51-47bc-a0f9-5d1205de4325

📥 Commits

Reviewing files that changed from the base of the PR and between acfcc1a and 92db1fe.

📒 Files selected for processing (2)
  • packages/gooddata-eval/src/gooddata_eval/core/agentic/__init__.py
  • packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py

Comment thread packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/src/gooddata_eval/core/agentic/_langfuse.py`:
- Around line 75-78: Update _ObservationListResult.list to paginate through all
observation pages instead of limiting retrieval to the first 100, using
/api/public/v2/observations with cursor pagination, io fields, and string I/O
decoding for Cloud and self-hosted v4 deployments. Preserve equivalent
pagination through the legacy /api/public/observations endpoint for self-hosted
v3, or explicitly enforce a supported-deployment constraint, so
_select_kda_trace() receives the complete observation set.
🪄 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

Run ID: 73b38461-b03f-45d1-8fc6-36e4eae5603c

📥 Commits

Reviewing files that changed from the base of the PR and between b4bf376 and 434f8f5.

📒 Files selected for processing (2)
  • packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.py
  • packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py

Comment thread packages/gooddata-eval/src/gooddata_eval/core/agentic/_langfuse.py Outdated
@FrankHuynh
FrankHuynh force-pushed the QA-28800-kda-skill branch 2 times, most recently from 02ac594 to 73f0c22 Compare August 5, 2026 09:43
FrankHuynh added a commit that referenced this pull request Aug 5, 2026
kda_skill.py shipped with zero test coverage, unlike its metric_skill/
alert_skill siblings which each have a dedicated test file -- this is
what tripped codecov/patch (27.91% vs 78.30% target) on PR #1706.
Covers the pure helpers, the KDA-trace selection/pagination logic,
classify_kda_report_bucket, and run_agentic_kda_skill/
evaluate_agentic_kda_skill via a mocked ChatClient, mirroring the
existing test_agentic_metric_skill.py/test_agentic_alert_skill.py
patterns. 94% coverage on kda_skill.py.

JIRA: QA-28800
risk: nonprod
Comment thread packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py Outdated
Comment thread packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py Outdated
@FrankHuynh
FrankHuynh force-pushed the QA-28800-kda-skill branch 4 times, most recently from 3124f15 to a1beca9 Compare August 6, 2026 14:50
Adds an agentic evaluation runner for the Key Driver Analysis (KDA) skill:
drives the create/execute KDA tool-call flow through a live chat session,
evaluates completion (kda_triggered/executed/success/turn_completed) plus
informational per-field correctness (Measure/Date Attribute/Periods/
Filters/Summary, all kda_-prefixed scores, not yet gated on strict_pass),
and logs whole-turn latency plus pass_at_k/pass_power_k for gdc-nas's
combo_report.py to bucket into its own daily-report table.

Trace selection is deliberately NOT attempted here: a conversation can
have several Langfuse traces sharing one session_id (title generation, a
disambiguation turn, the actual KDA turn), and picking the right one
requires checking observations, which are ingested asynchronously just
like latency. This module links whichever trace the default (max-latency)
selector finds, like every other skill; combo_report.py resolves the real
KDA trace itself, well after the run, when ingestion has settled -- and
owns that logic entirely rather than importing it from here, since the
cross-repo import never actually resolves in the report-generation job.

Also from PR review, including two follow-up independent re-reviews:
- log_quality_and_value_scores (used by every agentic skill, not just
  KDA) no longer treats an unresolved latency/cost as the worst possible
  outcome -- drops that weighted term and renormalizes instead.
- Dedupe _is_asking_clarification (copy-pasted across kda_skill.py,
  metric_skill.py, and a drifted third copy in conversation.py) into a
  shared _clarification.py. Only the bare "?"-anywhere check is
  tightened to require the message end on a question; the "could
  you"/"please"/"clarif" substring checks stay as broad as before, since
  they weren't the source of the original false-positive and
  conversation.py's multi-turn driver relies on their recall.
- All 6 informational KDA scores are kda_-prefixed, closing off any
  future collision with another skill's own score of the same shape.
- pass_power_k is now logged (mirrors visualization.py's own
  pass_at_K/pass_power_K) instead of being computed and discarded.
- Replaced an unconditional print() with _log.info, named the
  0.01-absolute-tolerance magic number, and fixed a couple of stale/
  missing comments (a docstring reference to a removed function, a
  missing copyright header).

JIRA: QA-28800
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.

2 participants