feat(security-issue-sync): track the post-advisory security-pages update - #1355
Conversation
The ASF security-committers policy requires the project's public security pages to be updated after an advisory ships, but no skill, process doc, or roles doc mentioned the step, so it was silently skipped in every advisory lifecycle the skills drive. The edit lives on the project website, outside everything sync can write, so sync cannot perform it and must not gate the close-out on it. Instead, both release-manager hand-off comment variants now carry a `Project security pages updated with CVE_ID` checkbox embedding the new security-pages-checklist marker; a new Step 1d signal row proposes a one-line reminder comment (at most once per tracker) when the advisory has shipped but the box is still unticked, and reading the same marker back in ticked form keeps later sync runs quiet. The new `security_pages_url` key in the project manifest template lets the reminder link the project's actual pages, and a new step-2b eval case (case-8) covers the proposal shape and the once-per-tracker guard. Fixes apache#180 Generated-by: ZCode (GLM-5.3-Flash)
There was a problem hiding this comment.
Great PR! The addition of the security_pages_url key perfectly aligns with Magpie's placeholder philosophy. The non-blocking reminder approach is also exactly how we want to handle off-tracker manual asks.
I was just reading through and noticed a tiny phrasing issue in signals-to-actions.md that might confuse the LLM during the parsing step.
You wrote:
Scan the issue body and every comment for
<!-- apache-magpie: security-pages-checklist v1 -->followed by a ticked box;
But based on the checklist item structure:
- [ ] <!-- apache-magpie: security-pages-checklist v1 --> Project security pages updated with CVE_ID
When ticked on GitHub, it becomes:
- [x] <!-- apache-magpie: security-pages-checklist v1 --> Project security pages updated...
Here, the ticked box ([x]) actually precedes the HTML marker, it doesn't follow it. Since agents rely on literal English instructions, looking for a box after the marker might cause the LLM to misread it as pending and spam the reminder.
Might be safer to rephrase it to: "Scan... for a ticked box (- [x]) preceding the marker" just to make it bulletproof for the LLM! Just an observation from my side.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for picking up #180 — the approach (checkbox in the hand-off, never gate the close-out) is the right one. Two issues mean the reminder can't work as written yet; details below and inline.
Blocking — the reminder is never evaluated (gather.md:362)
The Step 14 close-out adds announced and closes the tracker in the same apply, so the trigger state only ever exists on a closed tracker. Closed trackers only get the 1g cve.org check (bulk-mode.md: "Everything else is a no-op on closed issues"), so the new 1d row never runs. The check needs to move into the 1g closed-bucket sub-steps, with bulk-mode.md updated accordingly.
Blocking — trigger and guard contradict each other (signals-to-actions.md:1074)
"nor a comment carrying the marker" excludes the hand-off comment itself, which always carries the marker. And because the reminder reuses the same marker, the once-per-tracker guard can't tell the reminder from the hand-off. Suggest a distinct <!-- apache-magpie: security-pages-reminder v1 --> marker for the guard, with "satisfied" defined as any - [x] <!-- apache-magpie: security-pages-checklist v1 -->.
Hand-off PATCH-in-place resets the tick
The hand-off idempotency rule PATCHes the comment whenever its body differs from the variant sync would render. A ticked - [x] never matches the rendered - [ ], so a later variant flip (OAuth ↔ manual-paste) silently unticks the box and the reminder fires for a done step. The PATCH needs to preserve the checkbox state (or the comparison needs to ignore it).
Smaller observations
- The "You're done … no further comments will tag you" line in both hand-off variants is now contradicted by the reminder.
- case-8 covers only the positive path; the README claims the ticked / not-shipped / already-reminded paths too. Negative fixtures are needed, and the step-2b suite should be run (
AGENTS.md→ Keeping evals and mode-economics in sync). - The unset-
SECURITY_PAGES_URLfallback needs spelling out in the template, since the link markup is hardcoded and the skill is told to load the template verbatim. case-8/report.md:11uses the list index as Public advisory URL; a real value is an archived thread URL.
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.md.
…osed-tracker pass The close-out adds `announced` and closes the tracker in one apply, so the reminder's trigger only exists on closed trackers, which sync scans only in Step 1g. Move the check there, give the reminder its own marker so the once-per-tracker guard can tell it from the hand-off comment, keep the release manager's tick when the hand-off comment is re-rendered, and cover the no-reminder paths with two new eval cases. Generated-by: Claude Opus 5
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — pushed fixups for the review points; LGTM now.
This review was drafted by an AI-assisted tool and
confirmed by an Apache 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 Apache Magpie handles maintainer review:
CONTRIBUTING.md.
3 participants
Summary
Fixes #180. The ASF security-committers policy requires updating the project's public security pages after an advisory ships, but nothing in the skills,
process.md, orroles.mdmentioned the step, so it was silently skipped in every advisory lifecycle. Sync can't do that edit — it lives on the project website — so the skill tracks it on the tracker instead:Project security pages updated with CVE_IDcheckbox carrying an<!-- apache-magpie: security-pages-checklist v1 -->marker; ticking the box records the marker and later syncs read it as satisfied.projects/_template/project.mdgains asecurity_pages_urlkey so the reminder can link the project's actual pages (plain-phrase fallback when unset).Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md) — both hand-off comment variantsdocs/,README.md,CONTRIBUTING.md) —process.mdSteps 13/14/15,roles.mdrelease-manager sectionprojects/_template/) —security_pages_urlkeyTest plan
prek run --all-filespasses (also green in CI, lychee included)expected.jsoncleanly (PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner --quiet tools/skill-evals/evals/security-issue-sync/step-2b-proposed-changes/fixtures).case-8-security-pages-reminder(reminder fires once post-advisory; nothing before the advisory ships or once the marker is ticked).RFC-AI-0004 compliance
<project-config>,SECURITY_PAGES_URL);check-placeholdersis the mechanical gateLinked issues
Closes #180.
Notes for reviewers
Generated with ZCode (GLM-5.3-Flash)