fix(license-compliance-audit): handle large blobs and canonical SPDX - #1089
Conversation
Generated-by: OpenAI Codex (GPT-5.6)
Dropping case-insensitivity alongside whitespace normalisation went one step too far. The SPDX specification requires case-insensitive identifier matching — 2.3 Annex D says identifiers "should be matched in a case-insensitive manner. MIT, Mit and mIt should all be treated as the same identifier", and 3.x makes expressions case-insensitive throughout — so a header reading `apache-2.0` would have been reported WRONG-SPDX-HEADER at medium severity. That is a new false-positive class in a compliance audit, the same kind of defect this change set out to remove. Keep the punctuation and internal-whitespace strictness, which is the real fix: `Apache 2.0` is not a valid SPDX identifier and the previous wording wrongly treated it as equivalent to `Apache-2.0`. The eval fixture needed no change; case-5 exercises only the space form. Co-authored-by: Jarek Potiuk <potiuk@apache.org> Generated-by: Claude Code (Opus 5)
potiuk
left a comment
There was a problem hiding this comment.
Approving with one fixup pushed. The core of this is a real bug fix and the
diagnosis was right.
The large-blob bug was genuine, and worse than it looks
The old recipe was:
gh api repos/<upstream>/contents/<file_path> \
--jq '.content' | base64 --decode | head -10 | grep "SPDX-License-Identifier"For a blob over about 1 MiB the contents API returns encoding: "none" with no
inline content, so the decode produces nothing, grep matches nothing, and the
file is recorded as MISSING-SPDX-HEADER. That is a false compliance finding
against a file that is perfectly compliant — the worst failure mode an audit
skill has, because it sends someone to "fix" something that is not broken and
quietly erodes trust in every other finding in the report.
The principle you wrote into the skill is exactly the right one, and worth
quoting back:
An unavailable API response is never evidence that the source file lacks an
SPDX header.
Tracking those files as uninspected and keeping them out of the
missing/wrong counts is the correct fix, and case-5 pins it properly:
uninspected_spdx_count: 1 alongside missing_spdx_count: 0. Adding
uninspected_spdx_count to the output spec, with the explicit note that it must
not roll into finding_count, is what makes the distinction enforceable rather
than aspirational.
Scoping the /tmp/lca-missing-spdx.txt citation to local-checkout scans is also
a genuine correctness fix — a GitHub-API scan never writes that file, so the
report was pointing at a path that does not exist.
The fixup: SPDX matching stays case-insensitive
The one thing I changed. This PR replaced the old rule with "Do not normalise
punctuation, internal whitespace, or case". The whitespace half is right and was
the actual bug — the previous text claimed Apache-2.0 and Apache 2.0 are
equivalent, and Apache 2.0 is not a valid SPDX identifier at all. But dropping
case-insensitivity contradicts the SPDX specification:
SPDX 2.3, Annex D: license identifiers "should be matched in a
case-insensitive manner. MIT, Mit and mIt should all be treated as the same
identifier and referring to the same license."
SPDX 3.x goes further and makes expressions case-insensitive throughout,
operators included. So a header reading apache-2.0 would have been classified
WRONG-SPDX-HEADER at medium severity — a new false-positive class, which is
the same category of defect this PR set out to remove. Ironic enough to be worth
naming rather than silently fixing.
The fixup restores case-insensitivity with the spec citation, and keeps your
punctuation and internal-whitespace strictness intact. Your eval fixture needed
no change at all: case-5 exercises only the space form, so it was already
testing the correct behaviour.
Sources:
SPDX 2.3 Annex D,
SPDX 3.0.1 Annex B.
Nice first contribution — finding a false-positive path in a compliance audit is
a more valuable catch than the diff size suggests.
This review was drafted by an AI-assisted tool and
confirmed by a Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Magpie handles maintainer review:
CONTRIBUTING.md.
Summary
MISSING-SPDX-HEADERfindings, and report coverage counts explicitly.Apache-2.0SPDX identifier and keep the local/tmp/lca-missing-spdx.txtreference out of GitHub-scan reports.Apache 2.0token.Type of change
skills/<name>/) — eval fixtures updated belowTest plan
PATH="$PWD/.venv/bin:$PATH" prek run --all-filespasses, including workspace pytest, ruff, mypy, Markdown, link, skill, and spec checks.uv run --project tools/skill-and-tool-validator --group dev skill-and-tool-validatepasses (only pre-existing advisory warnings in unrelated skills).license-compliance-auditmodel eval cases pass.RFC-AI-0004 compliance
<upstream>placeholder and GitHub adapter path; no model vendor is introduced.Linked issues
Closes #944
Notes for reviewers
The raw-media request is used instead of decoding the JSON
contentfield because GitHub returnsencoding: "none"for blobs above the inline-content threshold. A failed raw fetch is deliberately surfaced as an audit coverage gap rather than converted into a compliance finding.Generative AI disclosure
Codex was used while preparing this change. The contributor reviewed the diff and test results before submission. The commit carries a
Generated-by: OpenAI Codex (GPT-5.6)trailer and no AICo-Authored-By:trailer.