Repository navigation
fix(security): deduplicate imports using authorized tracker body index - #1750
Conversation
|
@potiuk , Implemented the fix for #1730 and completed validation. The security-import workflow now uses deterministic matching against an authorized tracker body index instead of relying on GitHub issue search, which can miss backtick-wrapped thread IDs. Validation:
The implementation also fails closed when tracker completeness cannot be established, preventing unsafe duplicate imports. Thanks! |
potiuk
left a comment
There was a problem hiding this comment.
Replacing body search with a deterministic match on the authoritative field is the right fix for #1730, and the fail-closed handling of missing bodies, ambiguous fields and a full index is good — but the matcher still misses three identifier shapes the field legitimately records, which reintroduces the same duplicate-import false negative.
The index matcher misses identifiers the field records (tools/security-management/src/security_management/issue_import/dedup.py:166)
identity = threads.classify(field_value)
threads.classify picks the primary reply thread; it does not enumerate every identifier in the field, so three recorded shapes come back complete: true, untracked: [<id>] — the false negative this PR exists to prevent.
Three cases, all reproducible with the PR's own field() helper:
- Maildir ids. The maildir backend's file name goes through the same
Gmail thread `<threadId>`template (outcomes/mailing-thread.md), butthreads._THREAD_IDis[A-Za-z0-9_-]{6,}and stops at the first.—1700000000.M1P2.host,U=12:2,Sis never matched, thoughvalid_idsaccepts it. - Sentinel first line.
classifyreturns early onN/A — opened from public PR/N/A — imported from markdown filewithout reading the lines below, so a trackerissue-deduplicatemerged from a PR import and a mail import loses its mail thread ids. - Several roots.
classifykeeps only the firstRoot Message-ID;merge.pyunions both trackers' fields, so the second root is dropped.
Rather than reuse classify, scan every line of the field for every identifier shape valid_ids accepts (Gmail/maildir token with the _OPAQUE_ID character set, every /thread/<id>, every Root Message-ID), regardless of sentinels — and add a test for each case.
Flagged by codex (adversarial review); cross-checked.
Smaller observations
tools/security-management/README.md:122— nothing callsdedup-queries/dedup-attributeafter this PR; remove them (code, README rows, tests) here, or name the follow-up in the PR body- See inline comments on
plugins/magpie-security/skills/issue-import/existing-tracker-dedup.md:18.
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.
potiuk
left a comment
There was a problem hiding this comment.
Marking this as changes requested, so the PR's status matches where it stands: the points in my earlier review (#1750 (review)) are still open and need a change, or a reply on their threads. This review adds nothing new to 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 guide.
# Conflicts: # tools/security-management/README.md # tools/skill-evals/README.md
|
@KatalKavya96, I merged
|
… rows The recorded-thread scan ended an archive id only at `/`, `?`, `#` or the end of the line, so `…/thread/<id> (relayed)`, a markdown link and an autolink went unseen, and a trailing comma stayed on a bare Gmail id. Such a thread read as untracked and a second tracker was proposed. The id now also ends at whitespace or a closing bracket, and trailing prose punctuation is dropped, while the untrimmed spelling is kept too because a maildir name can end in `,`. The README edit had been made against an older copy and reverted rows main had moved on: `detect` back to "or", the `drafting` row, the ASF-only other-projects checks and the `advisory-archive` command. It is main's table again with only the `dedup-index` row changed, and the vetted-ops backend table counts its ten operations. Generated-by: Claude Opus 5
potiuk
left a comment
There was a problem hiding this comment.
Thanks, both points from the earlier review are fixed: the recorded-thread scan now reads every id the field holds, and the skill doc spells the vetted-ops command and the saved path correctly. Two gaps were left, so I pushed them as a fixup commit (e563654, merged with your main merge as e8bb39c) rather than send it back:
dedup.py: the archive-URL pattern ended an id only at/,?,#or the end of the line, so…/thread/<id> (relayed), a markdown link and an autolink went unseen, and a comma stayed on a bareGmail threadid. Such a thread read as untracked and a second tracker would be proposed, which is the case this PR exists to prevent. The id now also ends at whitespace or a closing bracket and trailing prose punctuation is dropped; the untrimmed spelling is kept as well, because a maildir name can end in,. New tests cover each shape, and two of them fail on the previous head.tools/security-management/README.md(and the plugin mirror): the edit had been made against an older copy and reverted rowsmainhad moved on, includingdetectback to "sender pattern or preamble regex" (the code still requires both), thedraftingrow, the ASF-only other-projects checks and theadvisory-archivecommand. It ismain's table again with only thededup-indexrow changed. The vetted-ops backend table now counts its ten operations.
Please pull before pushing again.
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 guide.
Summary
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 — all hooks green.uv run pytest/ruff check/mypypasses — verified through workspace hooks and targeted package tests.security-issue-import/step-2-dedup: 4/4 passed, 0 failed, 0 manual, 0 errored.dedup-indexresults.case-4-backtick-threadid-matchto reproduce the reported failure.issue-listoperation does not expose bodies.RFC-AI-0004 compliance
check-placeholderspasses.Linked issues
Fixes #1730.
Notes for reviewers (optional)
Implementation: The new
issue-import dedup-indexcommand reads a saved tracker index and compares identifiers against the authoritative Security mailing list thread field. A dedicatedissue-list-with-bodiesvetted operation requires explicit caller authorization.Safety: A missing body, ambiguous mailing-thread identity, or potentially truncated 1000-issue index prevents negative deduplication claims. Jira and local-files backends are explicitly unsupported by this new body-index operation rather than silently treated as having no matches.
Trade-off: The bounded index avoids relying on GitHub's inconsistent search indexing, but trackers with 1000 or more returned issues require a further completeness-safe solution. This PR prioritizes preventing duplicate security imports over automatically proceeding when completeness cannot be established.
Validation scope: The four model evaluations use mocked tool outputs; deterministic Python tests separately verify the matching logic and vetted-ops authorization boundaries.