feat(tools): add maven-artifact-verify and wire JVM artefact checks into release-verify-rc - #1415
liwenjie200543 wants to merge 12 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Solid, well-scoped start on #1173 with green CI — stdlib-only, offline, no subprocess or archive extraction, and the CodeQL double-report fix checks out. But check 1 can report a false PASS: an element inherited from a staged parent passes without the parent being read, and a missing licence/scm on a POM with no <parent> is downgraded to a warning. Both need fixing before this can gate an RC; details inline.
Also before merge
- Check 3 verifies only that companion signature/checksum files exist, not their contents (inline).
jvm_artefact_checksandjvm_digest_setin the template aren't read anywhere yet (inline).
Smaller observations
tools/maven-artifact-verify/README.md— the tool is ASF-policy-specific (ALv2 requirement, Incubator disclaimer), so per AGENTS.md → Labeling it should declare**Organization:** ASF. The SPDX header comment also appears twice.tools/spec-loop/specs/release-management-lifecycle.md:80— "Step 6b" is a step inside the skill, but the surrounding parenthetical uses lifecycle step numbers; drop it there.tools/dev/tests/test_check_duplication.py— an unrelated flaky-test fix; please split it into its own PR (the same fix is riding along in two other open PRs).- The test plan leaves the new eval suite unrun; please run
tools/skill-evalsforrelease-verify-rcand paste the summary. - Labels: this needs
family:release-managementandcapability:*— I'll add them.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md.
|
|
||
| | Key | Value | Notes | | ||
| |---|---|---| | ||
| | `jvm_artefact_checks` | `on` | `on` (default) — run Step 6b whenever the staged set contains jars or POMs; `off` — skip (e.g. the project stages jars only through a platform checked elsewhere) | |
There was a problem hiding this comment.
jvm_artefact_checks: off and jvm_digest_set are never read: Step 6b reads only jvm_companion_location and § Digest set, so adopters get settings that do nothing. Please wire both into Step 6b (skip when off; pass jvm_digest_set as --digests) or drop them. Also, the example value staged for jvm_companion_location turns ABSENT into FAIL, while an unset value means observation-only, so copying the template silently opts into the stricter mode. Please state the unset default explicitly.
potiuk
added
family:release-management
|
Thanks for the thorough review — all points addressed on the branch: Check 1 false PASS — elements now resolve against the full staged parent chain (cycle-protected, up through grandparents): the first ancestor declaring the element is judged as-is, so a staged parent carrying MIT (or an empty Check 3 contents — checksums are now verified with Template keys — Step 6b now skips when Template issue refs — dropped the framework-internal Smaller points — README declares Eval suite — the release-verify-rc fixtures were synced with the new semantics. One blocker on my side: the runner needs a model CLI and my |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — solid turnaround. Both blockers are properly fixed: the parent chain is walked with cycle protection, a staged MIT or <scm>-less parent now fails the child, and a parentless POM missing elements is a hard FAIL, each with a test. hashlib checksum verification, the jvm_artefact_checks / jvm_digest_set wiring, rglob, the README header and Organization line, the spec wording and dropping the rider are all resolved.
Left before merge:
<scm>inheritance is per-field (inline) — a child declaring only<scm><tag>is failed even when its parent suppliesurl/connection. That's a false FAIL on a correct POM, the case #1173 says must never happen.- Template default (inline) —
jvm_companion_locationstill shipsstagedin the Value column. - Eval run for
release-verify-rc— understood you're blocked on model credits; I'll run it on my side.
The other inline comments are non-blocking polish.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| if pom["developers"]: | ||
| return True, None | ||
| return False, "<developers> declared but empty" | ||
| scm = pom["scm"] |
There was a problem hiding this comment.
Maven merges <scm> per field — a child declaring only <scm><tag>v1.0</tag></scm> still inherits url/connection from its parent. Here any local <scm> is judged alone, so that child is a hard FAIL (<scm> declared without url or connection) — both when a staged parent supplies them and when the parent isn't staged (where it should be INHERITED-UNVERIFIED). Please resolve url/connection per field along the chain (nearest non-empty ancestor wins, INHERITED-UNVERIFIED if the chain is unstaged), with tests for a tag-only child with a staged and an unstaged parent.
| entries[key + "_detail"] = "element absent and no parent POM in the staged chain declares it" | ||
| else: | ||
| detail = "element absent and the parent chain is not fully staged locally; verify against the effective POM" | ||
| if chain_reason == "cycle": |
There was a problem hiding this comment.
A cyclic parent chain is a broken staged set (Maven refuses to build it), so it can't be "a correct POM inheriting from the ASF parent" — the case INHERITED-UNVERIFIED protects. I'd make cycle a FAIL.
| algorithm this Python's ``hashlib`` does not provide (the file is | ||
| still required, but its content cannot be checked offline). The | ||
| file is parsed leniently: the first whitespace-separated token is | ||
| the recorded digest, tolerating a trailing newline or a BSD-style |
There was a problem hiding this comment.
<digest> <filename> is the GNU coreutils format, not BSD-style. The real BSD/tagged form (SHA512 (file) = <hex>, from shasum --tag) and gpg --print-md output (still published by some ASF projects) give a false "checksum mismatch". Please fix the docstring and state the accepted format, or extract the hex digest leniently.
| "has yet to be fully endorsed by the asf", | ||
| ] | ||
|
|
||
| APACHE_LICENSE_NAME_RE = re.compile(r"apache\s+license.*2\.0", re.IGNORECASE) |
There was a problem hiding this comment.
APACHE_LICENSE_NAME_RE misses the SPDX id Apache-2.0 and The Apache Software License, Version 2.0 — those only pass today via the URL regex, so <name>Apache-2.0</name> with no <url> FAILs.
| | Key | Value | Notes | | ||
| |---|---|---| | ||
| | `jvm_artefact_checks` | `on` | `on` (default) — run Step 6b whenever the staged set contains jars or POMs; `off` — skip (e.g. the project stages jars only through a platform checked elsewhere) | | ||
| | `jvm_companion_location` | `staged` | `staged` — the companion `.pom` / `-sources.jar` / `-javadoc.jar` set is staged with the RC and must be present, each companion signed and checksummed; `nexus-staging` — the jars publish through the Nexus staging repository only, so a locally absent jar is an observation, not a failure; *(unset)* — same as `nexus-staging` | |
There was a problem hiding this comment.
Thanks for documenting the unset default — but the Value column still ships staged, so copying the template verbatim still opts an adopter into strict mode. Please make it *(unset)*, like jvm_digest_set.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| # The main jar lives in the POM's own directory (in Maven | ||
| # repository layout that is the versioned subdirectory, not | ||
| # the staged root). | ||
| main = pom_path.parent / f"{data['artifact_id']}-{data['version']}.jar" |
There was a problem hiding this comment.
artifact_id / version come from the staged POM and go straight into a path here, so a POM with <artifactId>../../x</artifactId> makes the tool stat and hash files outside the staging directory. Low impact, since nothing leaves the machine, but cheap to close: reject coordinates that don't match Maven's [A-Za-z0-9_.-]+ before building paths.
|
Ran the
(Run by an AI-assisted tool on behalf of an Apache Magpie maintainer, who confirmed this comment.) |
|
Thanks for running the suite and for the review. All three "left before merge" items are on the branch (783c328..8b65339), plus the four non-blocking polish comments:
|
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
LGTM on the latest iteration. All items from the previous review round are cleanly resolved:
- Per-field
<scm>inheritance (url/connection) with cycle detection. - Path traversal validation on
<artifactId>and<version>. grading-schema.jsonupdated with prose fields for the Step 6b eval judge.- All 43 package unit tests pass.
The branch currently has a merge conflict with main. Please rebase on latest main so this can be merged.
…nto release-verify-rc The JVM blocking checks agreed on in apache#1173 (checks 1-3) had no local, offline enforcement path: release-verify-rc Step 6 covered source tarballs only, so a staged RC could ship POMs without an ALv2 licence, a podling without its incubation disclaimer, or companion -sources/-javadoc jars missing their .asc signatures and checksums. Add tools/maven-artifact-verify, a stdlib-only Python adapter that verifies a staged directory: - Check 1: every .pom carries an ALv2 licence, <developers> and <scm>. Elements absent from the POM are resolved against a locally staged parent POM; when that cannot resolve they are reported as INHERITED-UNVERIFIED (WARN), not FAIL. - Check 2 (--podling): the incubation disclaimer inside <description>, accepting both the standard text and the DISCLAIMER-WIP variant from the Incubator distribution guide, tolerant of whitespace and line-wrapping. Gated by a DISCLAIMER file in the source tree, leaving the project_stage mechanism of apache#1172 untouched. - Check 3: each staged main jar has its -sources.jar and -javadoc.jar companions, each companion signed (.asc) and checksummed. packaging=pom is exempt, classified jars (-tests, -shaded, ...) are never mistaken for companions, and a main jar not staged locally (Nexus-only publication) is recorded as an ABSENT observation rather than a failure. Wire the tool in as release-verify-rc Step 6b: skipped cleanly when no JVM artefacts are staged, keeping the existing step numbering and every cross-reference to Steps 7-9 intact. release-build.md gains the matching jvm_artefact_checks configuration surface, the capability map, docs/release-management/spec.md and the spec-loop spec are synced, and a step-6b-jvm-artefacts eval suite (4 cases) covers the new skill behaviour. Refs apache#1173 Generated-by: WorkBuddy (GLM-5.3-Flash)
Adding tools/maven-artifact-verify to the workspace in the previous commit changed the member set, so `uv run --locked` in CI (skill-token- count and every other uv-based job) rejected the stale lockfile. Re-run `uv lock` with uv 0.12.17: the diff is the new member entry only (16 added lines, lock revision unchanged at 3, no version bumps). Generated-by: WorkBuddy (GLM-5.3-Flash)
…w tool The labeler config and the generated vendor-neutrality block are derived from the repo tree; adding tools/maven-artifact-verify made both stale (`test_committed_config_is_in_sync` and the three vendor-neutrality in-sync tests fail in CI). - .github/labeler.yml: `tools/maven-artifact-verify/**` joins the `substrate:release` label (one line, from tools/dev/generate-labeler-config.py). - docs/vendor-neutrality.md: the agent-harness section counts 29 substrate tools and lists maven-artifact-verify as harness-agnostic; skill sections are unchanged. Generated-by: WorkBuddy (GLM-5.3-Flash)
… tool check-doc-sync caught two hand-maintained figures the Step 6b addition made stale: - docs/setup/marketplace.md: magpie-release-management publishes ~1.4k always-on tokens (verify-rc grew by the Step 6b section). - tools/skill-evals/README.md: release-verify-rc is 22 cases across 8 suites with the new step-6b-jvm-artefacts suite. Generated-by: WorkBuddy (GLM-5.3-Flash)
…ompanions CodeQL flagged two alerts on the new package; one was a real bug. check_companions appended a PASS record in a loop `else` whose body has no `break`, so the else ran unconditionally: a companion missing its .asc (or checksum) was reported twice — once FAIL, once PASS. Nothing downstream misclassified (status aggregation takes the FAIL), but the JSON carried contradictory records for the same companion. Collect the missing files first and branch on the result; a new regression assertion pins one classification per companion. The second alert was cosmetic: the POM fixture built one XML attribute string by implicit concatenation inside a list literal; the parts are now joined with explicit `+`. Generated-by: WorkBuddy (GLM-5.3-Flash)
…e staged parent chain check_pom_entries reported PASS for an element absent from the POM itself whenever any parent POM was staged, without ever reading the parent: a child of a staged parent declaring MIT (or no licence at all) passed a blocking check, and the walk never went past the direct parent. A POM with no <parent> and a missing element got the INHERITED-UNVERIFIED warning, but nothing can be inherited there and Maven Central rejects such a POM. Resolution now walks the locally staged parent chain with cycle protection: the first ancestor declaring the element is judged as-is (the same _evaluate_element used locally), a complete chain that supplies nothing — including a parentless POM — is a hard FAIL, and only a chain that cannot be fully resolved offline stays INHERITED-UNVERIFIED. check_disclaimer shares the chain, so an inherited description is judged as-is too, and report details name the ancestor coordinate. Negative tests cover staged-parent-MIT, staged-parent-without-scm, parentless-missing-element, a two-level chain resolving through the grandparent, and a parent-chain cycle. check_companions verifies checksum files with hashlib against the companion jar's actual bytes (still offline); unknown digest algorithms degrade to presence-only with an explicit finding instead of a fake PASS. .asc signatures stay presence-only offline — the Step 6b recipe extends the Step 2 gpg --verify flow to the companions — and the README says so. A top-level-only glob misreported a staging directory in Maven-repository layout as a non-JVM artefact set, so the scan is rglob-based and the main jar resolves next to its own POM. Generated-by: WorkBuddy (AI agent)
release-build.md declared two keys nothing read: Step 6b now skips cleanly (stating the skip) when the section declares jvm_artefact_checks: off, and resolves the recipe's --digests from jvm_digest_set when it is set, falling back to the Digest set. The template documents the unset default for jvm_companion_location (observation-only, same as nexus-staging) so copying the example value staged is a deliberate opt-in to the stricter mode. Step 6b text now describes the parent-chain resolution and the companion checksum verification as the tool implements them, extends the paste-ready recipe with gpg --verify lines for companion .asc files, and drops the bare apache#1172/apache#1173 references from the adopter-facing template (a bare #NNN resolves against the adopter's tracker once the template is copied). The lifecycle spec keeps its lifecycle step numbers only. The eval fixtures' tool-report excerpts and the output spec are synced with the new detail strings and the jvm_digest_set rule, and the measured_tokens stamp is re-measured. Generated-by: WorkBuddy (AI agent)
A maintainer run of the release-verify-rc suite scored step-6b 1/4: cases 2-4 reached the right verdicts but failed on pom_findings / companion_findings, which the step's grading schema did not list as prose fields, so the model's one-line paraphrases of the tool report were compared verbatim against expected.json. The output spec defines both as one-line model-written findings, not verbatim tool output, so they belong to the semantic grader alongside paste_recipe and tool_report; status and step stay exact. The suite README's grading-methodology paragraph named only paste_recipe; state the mechanism instead.
…chain Maven merges <scm> per field: a child declaring only <scm><tag> still inherits url/connection from an ancestor. The tool judged any local <scm> in isolation, so such a child was a hard FAIL both when a staged parent supplied the fields and when the parent was unstaged (where INHERITED-UNVERIFIED is the honest answer) - a false FAIL on a correct POM, the case apache#1173 says must never happen. url/connection now resolve independently along the staged chain (nearest non-empty declaration wins; empty and tag-only declarations never fail a child on their own). Either field resolvable is a PASS; neither field anywhere in a complete chain is a hard FAIL. <licenses> and <developers> stay element-level.
…mplate The Value column shipped staged, so an adopter copying the template verbatim was opted into strict mode, where a locally absent jar is a FAIL. Unset matches the documented default (same as nexus-staging: an absent jar is an observation) and mirrors jvm_digest_set.
…ents Four small review follow-ups: - A cyclic parent chain is a FAIL, not INHERITED-UNVERIFIED: Maven refuses to build one, so it cannot be a correct POM inheriting from the ASF parent - the case the warning protects. Applies to check 1 (licences/developers), the <scm> per-field resolution and check 2. - Checksum files are parsed leniently: the hex digest is extracted by shape, so the BSD/tagged 'ALGO (file) = <hex>' (shasum --tag) and 'gpg --print-md' layouts match like the GNU coreutils one already did. The old docstring called the GNU layout BSD-style; fixed. - APACHE_LICENSE_NAME_RE also accepts the SPDX id 'Apache-2.0' and the legacy 'The Apache Software License, Version 2.0' wording, which previously only passed via the URL regex. - artifactId/version are validated against Maven's [A-Za-z0-9_.-] before a path is built from them, so a POM with <artifactId>../../x can no longer point the companion checks outside the staged directory; that POM's jar checks are skipped with a finding instead.
Rebasing onto main pulled the upstream edits to release-verify-rc into the merged file, so the generated stamps no longer describe it: recompute surface_hash and measured_tokens, and bump the agent-harness count to 30/30 now that maven-artifact-verify is part of the tree the count measures. Generated-by: WorkBuddy (AI agent)
8b65339 to
05e2af4
Compare
|
Rebased onto latest
Full CI is green on the new head (52/52 checks, including the pytest matrix, prek, and the token-count |
4 participants
Summary
tools/maven-artifact-verify, a stdlib-only, fully offline Python adapter implementing blocking checks 1–3 agreed on in release-verify-rc: validate rc jars #1173 (POM licence/developers/scm, podling incubation disclaimer, signed companion-sources.jar/-javadoc.jarsets) against a locally staged directory — no Nexus probing in this PR.release-verify-rcStep 6b (numbered to preserve every existing cross-reference to Steps 7–9), skipped cleanly for non-JVM RCs, plus the matchingjvm_artefact_checksconfiguration surface in therelease-build.mdtemplate.docs/release-management/spec.md, and the spec-loop spec; add astep-6b-jvm-artefactseval suite (4 cases) for the new skill behaviour.Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-filespasses (not run locally: the author'suvis below the repo floor andprekis unavailable in this environment; the repo'sskill-and-tool-validator,check-doc-sync,check-workspace-membersand the surface-hash/token-count checks were run individually and pass — happy to runprekin CI or on request)uv run pytest/ruff check/mypypasses (24 new tests + the package'sruff/ruff format/mypyall clean; run with an isolated Python 3.13 venv since the localuvis 0.11.7)(
PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/<skill>/) (fixtures added and JSON-validated; runner not executed locally)(a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md) —
step-6b-jvm-artefacts, 4 casesSKIP)RFC-AI-0004 compliance
<PROJECT>,<tracker>,<upstream>,<security-list>) used in all skill / tool prose (thecheck-placeholdersprek hook is the mechanical gate)Linked issues
Refs #1173 (PR 1 of the approved split: blocking checks 1–3, local artefacts only; check 4 / informational checks 5–7 are follow-up PRs)
Notes for reviewers (optional)
INHERITED-UNVERIFIEDis a WARN, not a FAIL. Check 1 resolves absent licence/developers/scm elements against a locally staged parent POM; when that is impossible (e.g. inheritance fromorg.apache:apacheoutside the staging dir) the POM is reported as inherited-unverified rather than failing a correct POM. Effective-POM verification would need Maven or network access, both out of scope for an offline read-only check.--podlingis gated on aDISCLAIMER/DISCLAIMER-WIPfile in the unpacked source artefact, not on aproject_stageconfig key — that mechanism is release-* family has no ASF incubator/podling handling (IPMC vote, DISCLAIMER, incubator dist paths, -incubating suffix) #1172's territory and this PR does not pre-empt it.ABSENTobservation, not a FAIL — staging jars in the Nexus staging repo instead of the local dir is the common ASF workflow. It escalates toFAILonly whenrelease-build.mddeclaresjvm_companion_location: staged.DISCLAIMER-WIPvariant, verbatim from the Incubator distribution/branding guides, tolerant of whitespace and line-wrapping..last-syncdrift:tools/spec-loop/.last-syncon this branch is 37 commits behind main (pre-existing upstream drift); this PR does not bump it.🤖 Generated with WorkBuddy