Skip to content

add License headers as a first-class review category#187

Merged
potiuk merged 1 commit into
apache:mainfrom
justinmclean:check-headers
May 17, 2026
Merged

add License headers as a first-class review category#187
potiuk merged 1 commit into
apache:mainfrom
justinmclean:check-headers

Conversation

@justinmclean

@justinmclean justinmclean commented May 17, 2026

Copy link
Copy Markdown
Member

Problem

The pr-management-code-review skill had no "License headers" category. A PR that added a new source file without the standard Apache license header would pass the skill's Step 4 scan entirely unchecked — no finding, no REQUEST_CHANGES disposition.

This was confirmed by cross-checking the skill's category list against the ASF source headers policy (https://www.apache.org/legal/src-headers.html), which requires every ASF source file to carry the standard Apache header. The requirement existed in ASF policy but had no corresponding check in the review framework.

As a secondary finding during the fix, review-flow.md Step 4's hardcoded category list was also out of sync with criteria.md — it enumerated 8 categories while criteria.md listed 11. The missing three ("Quality signals to check", "Commits and PRs", "Security model") have been added in the same pass.

Changes

.claude/skills/pr-management-code-review/criteria.md

  • Added License headers to the canonical category list (between "Code quality" and "Testing"), matching the ordering in review-flow.md.

.claude/skills/pr-management-code-review/review-flow.md

projects/_template/pr-management-code-review-criteria.md

  • Added a "License headers" row to the Section anchors table with the ASF policy URL (https://www.apache.org/legal/src-headers.html) as the default anchor.
  • This is the URL the skill resolves at finding time to link contributors directly to the policy. Because this is a global ASF requirement — not project-specific — the URL ships as a ready-to-use default rather than a blank placeholder, so adopters don't need to fill it in.

Testing

Structural validation

The tools/skill-validator suite (test_real_repo_passes) was run against all SKILL.md files post-change. Result: 1 passed, 0 failed.

Functional dry-run against synthetic PR content

The skill was executed in dry-run mode against a hand-constructed PR diff:

  • PR adds two new files: airflow/utils/dag_bag_utils.py (missing license header) and tests/utils/test_dag_bag_utils.py (header present and correct).
  • Step 4 walked all 13 categories. At category tools(oauth-draft): port Gmail OAuth helpers into framework #4 ("License headers"), the skill detected the missing header on dag_bag_utils.py:1, raised a major finding citing https://www.apache.org/legal/src-headers.html, and generated a suggestion block with the correct header text.
  • The test file with the header present produced no finding (no false positive).
  • Disposition auto-picked REQUEST_CHANGES (1 major finding).
  • The draft review body included the finding with verbatim rule quote, diff excerpt with arrow at the offending line, and a GitHub suggestion block the contributor can apply in one click.
  • Before this fix, the same diff produced zero findings under this category because the category did not exist.

@justinmclean justinmclean requested a review from paulk-asert May 17, 2026 00:02
@justinmclean justinmclean self-assigned this May 17, 2026
@potiuk potiuk merged commit fdf1b22 into apache:main May 17, 2026
12 checks passed
@paulk-asert paulk-asert mentioned this pull request May 17, 2026
Merged
paulk-asert added a commit to paulk-asert/airflow-steward that referenced this pull request May 17, 2026
@paulk-asert
add License headers rule body: defer to header tooling, cover the no-…
82ac020 
…tooling and exclusion-masking cases

PR apache#187 added "License headers" to the canonical category list and
a section-anchor URL, but no rule body — making it the only
framework-default category with neither an adopter source nor a
criteria.md section. This adds the missing `## License headers`
section so it matches its siblings (Third-party license compliance,
Security model, Quality signals).

The section is written as a deference-plus-judgement layer, not a
re-implementation of apache-rat / pre-commit:

- When a header check is in the status-check rollup, that tool is
  authoritative for mechanical presence/absence; Golden rule 8 +
  the CI precheck already block approve on its failure. The skill
  does not raise a duplicate "missing header" finding, and treats
  the project's tool config (not the policy page) as the source of
  truth for scope/exclusions.
- Projects with no header tooling: the skill is the safety net and
  scans added/rewritten source files itself.
- Exclusion-masking case: when the same PR both adds a header-tool
  exclusion entry and a file lacking an Apache header, CI passes
  green by construction and deference gives no coverage — the skill
  raises a finding asking the maintainer to confirm the exclusion
  is appropriate.
- Judgement cases the tool cannot decide (mis-applied SPDX, wrong
  license, third-party header routing) are called out, closing the
  loop with the existing Third-party-license-compliance cross-ref.

Generated-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
potiuk pushed a commit that referenced this pull request May 17, 2026
@paulk-asert
add License headers rule body: defer to header tooling, cover the no-…
ba104c4 
…tooling and exclusion-masking cases (#195)

PR #187 added "License headers" to the canonical category list and
a section-anchor URL, but no rule body — making it the only
framework-default category with neither an adopter source nor a
criteria.md section. This adds the missing `## License headers`
section so it matches its siblings (Third-party license compliance,
Security model, Quality signals).

The section is written as a deference-plus-judgement layer, not a
re-implementation of apache-rat / pre-commit:

- When a header check is in the status-check rollup, that tool is
  authoritative for mechanical presence/absence; Golden rule 8 +
  the CI precheck already block approve on its failure. The skill
  does not raise a duplicate "missing header" finding, and treats
  the project's tool config (not the policy page) as the source of
  truth for scope/exclusions.
- Projects with no header tooling: the skill is the safety net and
  scans added/rewritten source files itself.
- Exclusion-masking case: when the same PR both adds a header-tool
  exclusion entry and a file lacking an Apache header, CI passes
  green by construction and deference gives no coverage — the skill
  raises a finding asking the maintainer to confirm the exclusion
  is appropriate.
- Judgement cases the tool cannot decide (mis-applied SPDX, wrong
  license, third-party header routing) are called out, closing the
  loop with the existing Third-party-license-compliance cross-ref.

Generated-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@justinmclean justinmclean deleted the check-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

Reviewers

@potiuk potiuk potiuk approved these changes
@paulk-asert paulk-asert Awaiting requested review from paulk-asert

Assignees

@justinmclean justinmclean

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@justinmclean @potiuk