Add api-contract-audit skill for type hint checking - #962
Rimsha2535 wants to merge 5 commits into
Conversation
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
| - #940: Added shared validation for packaged agent skills and the `skills:check` Nox session. | ||
|
|
||
| ## Summary | ||
|
|
There was a problem hiding this comment.
Oh no, I'm not certain what happened here, but many of these lines come from
https://github.com/exasol/python-toolbox/blob/main/doc/changes/changes_10.5.0.md
I believe that you're file should have only:
# Unreleased
## Features
* #940: Added shared validation for packaged agent skills and the `skills:check` Nox session.
* #942: Added api-contract-audit skill for identifying mismatches between type annotations, docstrings, and runtime behaviorThere was a problem hiding this comment.
I only added the #942 entry. The extra content was already there when I created the PR, the conflict appeared at that point and the extra content crept in during resolution.
GitHub Actions
Waiting
GitHub Actions
Waiting
|
There was a problem hiding this comment.
This skill is packaged under exasol.toolbox.skills and is discovered by skills:check, but skills:install currently installs only exasol-python-toolbox.
How is a user expected to install api-contract-audit?
Please either extend the installer to support this skill or move/document it as a separately installed skill.
There was a problem hiding this comment.
I agree that api-contract-audit should remain as a separate skill under exasol/toolbox/skills/api-contract-audit/SKILL.md, since it is generic and not part of the exasol-python-toolbox skill. The current limitation is that skills:install only installs the PTB skill, while skills:check already discovers all packaged skills. I would keep this location and rely on the installer PR to add support for installing individual or all packaged skills, rather than move this skill under exasol-python-toolbox.
There was a problem hiding this comment.
Since you are already handling the installer in #960, should I add documentation for manually installing this skill in the meantime?
There was a problem hiding this comment.
We should merge @jana-selva 's first & then adapt the skills:install and documentation in this PR.
| prompt: "Audit the PyExasol public API for mismatches between type annotations and runtime behavior." | ||
| expected: | ||
| must_include: | ||
| - "isinstance" |
There was a problem hiding this comment.
Requiring isinstance makes this evaluation dependent on one implementation detail.
Could this expectation be generalized to runtime validation/behavior evidence, so valid findings based on type(), coercion, or other checks are also accepted?
There was a problem hiding this comment.
@jana-selva I included isinstance as it is explicitly listed as a typical search signal in the skill itself. But I am happy to generalize it if needed.
| @@ -0,0 +1,25 @@ | |||
| version: 1 | |||
There was a problem hiding this comment.
Is a skill-specific test needed to validate that this skill’s resource and evaluation cases are available and correctly structured?
The existing tests appear to cover only exasol-python-toolbox; skills:check validates general Markdown rules but not these evaluation cases.
There was a problem hiding this comment.
Good point. Should I add skill-specific tests similar to the ones for exasol-python-toolbox or is there a preferred pattern to follow for new skills?
There was a problem hiding this comment.
Yes, please add skill-specific tests similar to the existing exasol-python-toolbox tests.
At minimum, verify that SKILL.md and eval_cases.yml are available, the frontmatter is valid, and the evaluation cases contain the expected structure. This will also establish a reusable pattern for future skills.
There was a problem hiding this comment.
Ah, to be fair, some of these are already covered by the skill:check. (I'd verified that in my initial review). Perhaps, it makes more sense to validate that this particular file is included in a test check - right now we mock it, so I would suggest changing that as otherwise we are duplicating tests & could end up with minor differences over time. This would then be an integrated version similar to what we have in nature to the cookiecutter tests.
3 participants
Fixes #942
Summary
Added the
api-contract-auditskill to help identify mismatches between type annotations, docstrings and runtime behavior in Python libraries. The skill was verified against PyExasol and works as expected.Checklist
Note: If any of the items in the checklist are not relevant to your PR, just check the box.
For any Pull Request
Is the following correct:
When Changes Were Made
Did you:
When Preparing a Release
Have you: