Skip to content

correct license-header criteria and add eval suite#205

Merged
potiuk merged 4 commits into
apache:mainfrom
justinmclean:tweak-headers
May 18, 2026
Merged

correct license-header criteria and add eval suite#205
potiuk merged 4 commits into
apache:mainfrom
justinmclean:tweak-headers

Conversation

@justinmclean

Copy link
Copy Markdown
Member

Follow-up to #195: fix license-header criteria and add eval suite

PR #195 added the ## License headers section to criteria.md. This PR corrects several issues found during review and adds a full behavioral eval suite covering the skill's Step 3, 4, and 6 sub-checks.

Criteria fixes

Correctness fixes

  • The severity table's last row incorrectly said `nit` / no finding — changed to no finding.
  • A table row was missing for the most common case: header tooling present, CI green, no exclusion change in this PR → no finding. Without it the table was silent on the dominant path.
  • The forward reference to "exemptions below" pointed at nothing — changed to "the Exemptions paragraph at the end of this section".

Calibration fixes

  • "Most ASF projects enforce headers in CI" overstated the prevalence — changed to "Many ASF projects".
  • Added the overly-broad exclusion case: when a PR adds an exclusion pattern wider than necessary (e.g. a whole subtree rather than a specific file), raise a minor finding even if the files in this PR carry correct headers, because the pattern silently degrades the tool for all future PRs.

Exemption gaps

Added exemptions that were missing from the original list:

  • Files in formats that do not support comments (JSON, CSV, most binary data) — cannot carry a header.
  • Documentation and plain-text files (.md, .rst, .txt) — ASF projects are conventionally lenient.
  • README files in any format.
  • LICENSE, NOTICE, DISCLAIMER, and similar legal declaration files — these are the licence artefact and must not carry an Apache header.

Category A attribution rule

The previous wording stated that a licenses/ directory entry and a LICENSE update were both expected. This is incorrect: a licenses/ directory is a common good practice but is not required by ASF policy. The sole determining factor is whether LICENSE or LICENSE.txt was updated with an attribution notice.

Eval suite

Adds tools/skill-evals/evals/pr-management-code-review/ — 36 cases across 6 suites covering the skill's deterministic sub-checks:

Suite Cases What it covers
step-3-security-disclosure-scan 6 CVE/security-phrase detection in title, body, commits; prompt-injection resistance
step-4-third-party-license 6 X/B/A classification; LICENSE update required; licenses/-only → major
step-4-compiled-artifacts 5 .jar/.pyc/.so/.whl detection; major vs blocking escalation
step-4-image-ip 4 Diagram vs polished-logo judgement; screenshot exemption
step-4-license-headers 8 Tooling deference; exclusion masking; overly-broad exclusions; no-tooling fallback; wrong SPDX; format/README/legal-file exemptions
step-6-disposition 6 APPROVE / REQUEST_CHANGES / COMMENT auto-pick logic

Currently all test pass

@justinmclean justinmclean requested a review from paulk-asert May 18, 2026 03:55

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pending "prek"

@justinmclean

Copy link
Copy Markdown
Member Author

yep just fixing that now -issue is mpostly that the example can't follow the linting rules we have

@potiuk

potiuk commented May 18, 2026

Copy link
Copy Markdown
Member

yep just fixing that now -issue is mpostly that the example can't follow the linting rules we have

AH.. We should make some rules for exclusions then in AGENTS.md

@paulk-asert paulk-asert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I hadn't thought about whether an empty jar should be allowed or not in a src distribution. We obviously don't want any opaque binaries. I can see a case for also banning empty jars. And I can see a case for a more lenient interpretation that perhaps allowed a jar containing fully readable javadoc html. But I am happy with what is captured now as a starting point - all these files will evolve once the rubber hits the road.

@justinmclean

justinmclean commented May 18, 2026

Copy link
Copy Markdown
Member Author

Empty jars do exist in existing projects. I've also come across jars that contain no code, but that's very rare.

@potiuk

potiuk commented May 18, 2026

Copy link
Copy Markdown
Member

I hadn't thought about whether an empty jar should be allowed or not in a src distribution. We obviously don't want any opaque binaries. I can see a case for also banning empty jars. And I can see a case for a more lenient interpretation that perhaps allowed a jar containing fully readable javadoc html. But I am happy with what is captured now as a starting point - all these files will evolve once the rubber hits the road.

Yeah. We also have almost empty - just metadata .whl files (which are also .zip files BTW.

@potiuk potiuk merged commit 4ff0c33 into apache:main May 18, 2026
13 checks passed
@justinmclean justinmclean deleted the tweak-headers branch May 28, 2026 00:29
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.

3 participants