fix(gooddata-eval): drop dead 'skills' fallback spelling in set_skills parsing

[cobanfurkanx]

Closes #1780

Description

As described in #1780, _set_skills_declarations() in packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py hedged across two argument spellings: skill_names and skills.

Since skill_names is the authoritative field declared by the set_skills tool and emitted by the platform, the legacy fallback to skills was dead and allowed tests to pass with payloads the platform never emits.

Changes

• Removed the fallback to args.get("skills") in _set_skills_declarations().
• Updated all remaining occurrences of "skills" as an argument key in test_agentic_conversation.py to use "skill_names" consistently.

Summary by CodeRabbit

• Updates
  • Skill declarations now read from the skill_names field. The legacy skills field is no longer used as a fallback, so requests without skill_names are treated as having no declared skills.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f3a238a4-78a1-46c2-9a80-97aa82d81383

📥 Commits

Reviewing files that changed from the base of the PR and between 8168136 and ca7aa47.

📒 Files selected for processing (2)

• packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py
• packages/gooddata-eval/tests/test_agentic_conversation.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

_set_skills_declarations now reads skill names only from skill_names. Conversation tests use skill_names in their set_skills payloads.

Changes

Skills declaration parsing

Layer / File(s)	Summary
Parse and test skill declarations packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py, packages/gooddata-eval/tests/test_agentic_conversation.py	The parser no longer falls back to skills when skill_names is absent. Conversation tests now use skill_names in their payloads.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ca7aa

In-repository callers and tests use skill_names, and the available evidence shows no concrete regression from removing the fallback. No actionable merge-blocking risk is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ca7aa

The change affects reported skill activation and evaluation outcomes, without an identified expansion of permissions or attacker reachability. Compatibility remains uncertain because the external tool declaration was not available to verify the canonical argument key.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated impact is bounded to skill attribution and conversation evaluation results. No PR-induced tenant, workspace, credential, or tool-authority expansion was identified; downstream uses of these results are not established.

Trust Boundaries and Controls

• observed — Producer-supplied argument names pass through the unchanged JSON decoder into the declaration parser. The removed alias is a compatibility interpretation, not an authentication or authorization control. Whether the external producer ever relies on that alias remains unverified.

Resilience and Maintainability Implications

• observed — Existing lifecycle containment is unchanged: supplied conversation IDs remain caller-owned, created conversations are cleaned up in finally, and partial ChatError events remain available for declaration processing and resource cleanup. The alias removal changes event interpretation, not ownership or recovery ordering.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: removing the legacy "skills" fallback from set_skills parsing.
Linked Issues check	✅ Passed	Issue #1780 requires removal of the legacy skills fallback and correction of test argument keys. _set_skills_declarations() now reads only skill_names. The changed conversation tests now use `sk…
Out of Scope Changes check	✅ Passed	The changes stay within issue #1780. They update the targeted parser and its documentation, plus related conversation test payloads. No unrelated production behavior or test area changed.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{A rabbit checked the skill-name key,
And found the fallback gone.
The tests now send skill_names,
As conversations carry on.
The carrot patch approves the change,
And hops beneath the dawn.}

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