fix(gooddata-eval): retry KDA on any non-triggering response, drop text classification - #1733
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe KDA skill now sends a simulated reply after each non-empty, non-final response. It supports metric and period hints, raises the default iteration limit to four, and retains retry termination on failure. Tests cover metric and period clarification flows. ChangesKDA simulated clarification handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR improves clarification handling for option lists and period questions, with targeted end-to-end coverage and reported checks passing. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant KDAEvaluator
participant ExpectedOutput
participant SimulatedResponseGenerator
participant KDAAgent
KDAEvaluator->>KDAAgent: submit initial KDA request
KDAAgent-->>KDAEvaluator: return non-final response
KDAEvaluator->>ExpectedOutput: derive metric and period hints
ExpectedOutput-->>KDAEvaluator: return available context
KDAEvaluator->>SimulatedResponseGenerator: generate simulated reply
SimulatedResponseGenerator-->>KDAEvaluator: return follow-up response
KDAEvaluator->>KDAAgent: submit simulated reply
KDAAgent-->>KDAEvaluator: return final KDA result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gooddata-eval/tests/test_agentic_kda_skill.py (1)
117-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd numbered-option regression cases.
The requirement includes numbered option lists, but these tests cover only
-bullet markers. Add positive cases for both1.and1)formats, which_LIST_ITEM_REaccepts.🤖 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/tests/test_agentic_kda_skill.py` around lines 117 - 138, Extend the clarification detection tests around _is_asking_kda_clarification with positive regression cases whose option lists use both “1.” and “1)” markers. Keep the question followed by the numbered list at the end of the message, and assert the helper returns true, covering the two formats accepted by _LIST_ITEM_RE.
🤖 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.
Nitpick comments:
In `@packages/gooddata-eval/tests/test_agentic_kda_skill.py`:
- Around line 117-138: Extend the clarification detection tests around
_is_asking_kda_clarification with positive regression cases whose option lists
use both “1.” and “1)” markers. Keep the question followed by the numbered list
at the end of the message, and assert the helper returns true, covering the two
formats accepted by _LIST_ITEM_RE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c3772f6-7323-4def-bfc1-9ce4eb1da797
📒 Files selected for processing (2)
packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.pypackages/gooddata-eval/tests/test_agentic_kda_skill.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1733 +/- ##
==========================================
+ Coverage 79.42% 79.44% +0.02%
==========================================
Files 272 272
Lines 18997 19012 +15
==========================================
+ Hits 15088 15105 +17
+ Misses 3909 3907 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
67f6c97 to
3a76027
Compare
FrankHuynh
changed the title
myhoai
left a comment
myhoai
left a comment
There was a problem hiding this comment.
Reviewed the full core/agentic/ package to put this fix in context. The change is correct and minimal — the new branch matches the captured trace, and none of the five existing _is_asking_kda_clarification tests regress. Two things I'd like to discuss before merge.
1. The cost of the two error directions is very asymmetric, and the heuristic is biased the wrong way
_is_asking_kda_clarification is only ever reached when create_args is None (kda_skill.py:226 already broke out otherwise), i.e. only on runs that are currently failing.
- False negative (agent is asking, we think it answered) → break after turn 1 →
triggered=False→ hard failure. That's QA-28800. - False positive (agent answered, we think it's asking) → one extra turn + one gpt-4o-mini call, plus noise on
kda_disambiguated. It cannot turn a pass into a fail, because the loop already broke oncecreate_argswas set.
So false negatives are expensive and false positives are nearly free — but the matcher is written strict-first (endswith("?")), which optimises for the cheap direction.
I ran the post-fix heuristic against neighbouring response shapes:
| Response shape | Result |
|---|---|
Which one?\n- a\n- b (the captured trace) |
✅ True |
Which one?\n- a\n- b\n\nLet me know which one you prefer. |
❌ False |
| question + list where an item wraps onto an indented continuation line | ❌ False |
• or – bullets instead of -/* |
❌ False |
| options rendered as a markdown table | ❌ False |
**Option 1** … (bold, no space after *) |
❌ False |
Row 2 — question, option list, one closing sentence — is a very common LLM shape and is my best guess at the next ticket. The fix patches one shape; the adjacent ones still fail identically.
2. Every other skill in this package solves this differently — KDA is the only outlier
| Module | Break condition | "Is it asking?" detection | Budget |
|---|---|---|---|
general_question, guardrail, search_tool |
— | none, single turn | 1 |
visualization.py:165-179 |
viz produced, or empty text_response |
none — never classifies | 4 |
alert_skill.py:455-474 |
tool_called, or empty response with no tool calls |
none — never classifies | 6 |
metric_skill.py:168-172 |
metric created | "?" in t or "could you" or "please provide" or "clarif" |
7 |
conversation.py:195-199, 330 |
turn ends | same but looser ("please"), plus the structural chat_result.alert_proposals signal |
20 |
kda_skill.py:236 |
create called | endswith("?") + the new list branch |
3 |
visualization and alert_skill — the two closest analogues (run a skill, produce an artifact, may need a round trip) — deliberately never classify the text at all. They break on the goal signal (viz created / alert tool called) and on an empty response, and otherwise just nudge again. That's exactly the asymmetric-cost reasoning above, and it makes this whole class of bug structurally impossible.
KDA already has the equivalent goal signal (create_args is not None, kda_skill.py:226). That makes _is_asking_kda_clarification a pure extra early-give-up condition — dropping it turns KDA into the viz/alert model.
Suggestion: rather than adding a second regex rule, consider matching visualization.py:165-179: break on create_args is not None or empty response_text, otherwise always send a simulated reply. That fixes QA-28800 and every shape in the table above, with no further tickets. If you keep the classifier, at minimum please widen _LIST_ITEM_RE and tolerate a trailing prose line. Either way _DEFAULT_MAX_ITERATIONS = 3 deserves a bump — kda_skill.py:18-20 says 3 is already sized exactly for "metric and period each needing their own question", so there is no slack for a wasted turn (viz/alert/metric carry 4–7).
If there is a KDA-specific reason it must classify where viz and alert don't, that's worth a line in the docstring — the current "each skill's heuristic has already drifted independently" reads as license to drift, but the actual drift in this package is only between loose and absent.
Rest is nits, inline. Nice trace-backed problem statement in the description, and thanks for the period-hint plumbing — that gap was real.
3a76027 to
e68ef79
Compare
…xt classification _is_asking_kda_clarification tried to classify agent responses as "asking for clarification" vs "a final answer" via a "?"-based heuristic, so a simulated user reply was only sent when it matched. That heuristic missed a real, common response shape: a clarifying question immediately followed by a bullet list of the options being offered (e.g. "Which metric?\n- metric A\n- metric B") -- the message doesn't end on "?" itself, so the run gave up after turn 1 instead of ever nudging the simulated user to pick one, scoring a genuinely-ambiguous case as triggered=False. Found via a real CI trace (QA-28800, gpt56luna_openai / globalmart): the chatbot asked to disambiguate between two "Total Net Revenue" metrics -- one of which was the expected answer -- but kda_disambiguated stayed False and the session never got a second turn, confirming the simulated-reply path was never reached. First pass patched the heuristic (wider bullet-char support, "?" no longer needing to be the literal last character, a period_hint gap, a "None 'None'" prompt bug). Review (chi My) pointed out the cost of the two error directions is asymmetric: missing a genuine clarifying question hard-fails the run, while misreading a final answer as one only costs one harmless extra turn (the loop already breaks for good once create_args is set, so a false positive here can never turn a pass into a fail). Every other skill in this package (visualization.py, alert_skill.py) already solves this the cheap way: never classify the text at all, just break on the goal signal (tool called / artifact produced) or an empty response, and otherwise always retry. Patching the KDA-specific heuristic for one more response shape (this round it was "**Option 1**: ..." -- bold markdown with no space after the marker) would have meant chasing an open-ended list of shapes forever. Fix: dropped _is_asking_kda_clarification and _LIST_ITEM_RE entirely. _run_once now matches visualization.py/alert_skill.py's own break conditions -- create_args set, or an empty response -- and otherwise always sends a simulated reply, regardless of what the agent's text says or how it's formatted. _DEFAULT_MAX_ITERATIONS bumped 3 -> 4 (chi My's point: 3 was sized exactly for 2 real questions with zero slack for a wasted turn; every other skill in the package budgets 4-7). Also fixed along the way: - generate_simulated_kda_response only ever knew about measure candidates, even when the agent's question was about the PERIOD to compare instead -- it had nothing period-specific to answer with. Extracted into _build_period_hint(), built from whichever of expected_output's Date Attribute/Analyzed Period/Reference Period fields are present (not requiring all three). - The prompt asserted "an acceptable metric/fact is None 'None'" as a real option when measure_candidates was None/empty (e.g. a period-only question) -- likely to make gpt-4o-mini invent a metric literally named "None". Extracted into _build_clarification_prompt(), which now omits the "For reference, ..." clause entirely when there's nothing usable to reference. Tests: _is_asking_kda_clarification's own unit tests removed along with the function; the end-to-end run_agentic_kda_skill regression tests for the real captured trace and the period-clarification case stay (now exercising the always-retry path instead of a classifier match), plus a new test for the bold- markdown case chi My's review flagged, direct unit tests for _build_period_hint and _build_clarification_prompt, and a bumped _DEFAULT_MAX_ITERATIONS. 48 tests in test_agentic_kda_skill.py, all passing; package suite unchanged at 9 pre-existing unrelated failures (missing openai module in this venv). JIRA: QA-28800
e68ef79 to
931817c
Compare
FrankHuynh
changed the title
|
Thanks for the deep dive — went with the redesign, not another patch.
Also bumped Left the module docstring's "each skill's heuristic has already drifted independently" line out since there's no more KDA-specific heuristic to justify. 48 tests passing (down from before since |
2 participants
What
_is_asking_kda_clarificationtried to classify agent responses as "asking for clarification" vs "a final answer" -- a simulated user reply was only sent when the text matched. That heuristic missed a real, common response shape: a clarifying question immediately followed by a bullet list of the options being offered, e.g.:The message doesn't end on
"?"itself (it ends on the list), so the harness treated this as a final answer and gave up after turn 1 -- never nudging the simulated user to pick an option. The case scoredtriggered=False, a hard failure, even though one of the two offered metrics was the expected answer.How found
Real CI trace (QA-28800,
gpt56luna_openaicombo,globalmartworkspace): traceba72a6d2c19a3ceeaa354ec641bc52f3.kda_disambiguatedstayedFalseand the Langfuse session had only 1 trace total -- confirming the simulated-reply path was never reached.Fix (redesigned per review)
A first pass patched the heuristic directly (wider bullet-marker support, "?" no longer needing to be the literal last character, plus a
period_hintgap and aNone 'None'prompt bug fixed alongside). Review (chi My) pointed out the cost of the two error directions here is asymmetric: missing a genuine clarifying question hard-fails the run, while misreading a final answer as one only costs one harmless extra turn -- the loop already breaks for good oncecreate_argsis set, so a false positive can never turn a pass into a fail. Every other skill in this package (visualization.py,alert_skill.py) already solves this the cheap way: never classify the text at all, just break on the goal signal (tool called / artifact produced) or an empty response, and otherwise always retry. Patching the KDA-specific heuristic for yet another response shape (this round:**Option 1**: ..., bold markdown with no space after the marker) would have meant chasing an open-ended list of shapes forever._is_asking_kda_clarificationand_LIST_ITEM_REare removed entirely._run_oncenow matchesvisualization.py/alert_skill.py's own break conditions --create_argsset, or an empty response -- and otherwise always sends a simulated reply, regardless of what the agent's text says or how it's formatted._DEFAULT_MAX_ITERATIONSbumped3 -> 4(3 was sized exactly for 2 real questions -- metric and period -- with zero slack for a wasted turn; every other skill in the package budgets 4-7).Also fixed along the way:
generate_simulated_kda_responseonly ever knew about measure candidates, even when the agent's question was about the period to compare instead -- it had nothing period-specific to answer with. Extracted into_build_period_hint(), built from whichever ofexpected_output'sDate Attribute/Analyzed Period/Reference Periodfields are present (not requiring all three)."an acceptable metric/fact is None 'None'"as a real option whenmeasure_candidateswasNone/empty (e.g. a period-only question) -- likely to make gpt-4o-mini invent a metric literally named "None". Extracted into_build_clarification_prompt(), which now omits the "For reference, ..." clause entirely when there's nothing usable to reference.Tests
_is_asking_kda_clarification's own unit tests removed along with the function. The end-to-endrun_agentic_kda_skillregression tests for the real captured trace and the period-clarification case stay (now exercising the always-retry path instead of a classifier match), plus:**Option 1**: ...)._build_period_hint(every field-presence combination) and_build_clarification_prompt(theNone-candidates fix).48 tests in
test_agentic_kda_skill.py, all passing.Verification
ruff check/ruff format --check: clean.pytest packages/gooddata-eval/tests/: 349 passed, same 9 pre-existing unrelated failures (missingopenaimodule in this venv) as onmaster.Not included in this PR
JIRA: QA-28800
Summary by CodeRabbit