Skip to content

fix(security): deduplicate imports using authorized tracker body index - #1750

Merged
potiuk merged 7 commits into
apache:mainfrom
KatalKavya96:fix/security-import-thread-dedup
Oct 31, 2026
Merged

potiuk merged 7 commits into
apache:mainfrom
KatalKavya96:fix/security-import-thread-dedup

Conversation

@KatalKavya96

Copy link
Copy Markdown
Contributor

Summary

  • Fixes security-issue-import: Step 2 thread-ID de-dup misses tracked threads (backtick-wrapped ids not found by search) #1730: GitHub issue search can miss security mailing-list thread IDs wrapped in Markdown backticks, causing previously imported reports to be treated as new.
  • Replace search-based exact-ID deduplication with deterministic matching against an authorized, bounded tracker issue-body index, covering Gmail IDs, PonyMail archive IDs, root Message-IDs, and secondary threads.
  • Fail closed on incomplete or ambiguous results, preserve least-privilege caller permissions, and add regression tests and updated evaluation fixtures.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other:

Test plan

  • prek run --all-files passes — all hooks green.
  • For Python packages touched: uv run pytest / ruff check / mypy passes — verified through workspace hooks and targeted package tests.
  • For Groovy bridges touched: command-line invocation tested end-to-end
  • For skill changes: eval suite passes for the affected skill.
    • security-issue-import/step-2-dedup: 4/4 passed, 0 failed, 0 manual, 0 errored.
  • For skill behaviour changes: a new or updated eval fixture is included in this PR.
    • Updated three existing fixtures to use dedup-index results.
    • Added case-4-backtick-threadid-match to reproduce the reported failure.
  • Other:
    • Tested exact identifier matching, PonyMail URLs, secondary threads, missing issue bodies, ambiguous identities, and truncated indexes.
    • Verified unauthorized callers are refused, explicitly authorized read operations are allowed, and the ordinary issue-list operation does not expose bodies.
    • Verified generated vetted-ops plugin mirrors match their source files.

RFC-AI-0004 compliance

  • HITL — any required caller-policy change requires explicit user approval; no new mutation is introduced.
  • Sandbox — tracker access remains restricted to the vetted-ops execution boundary; no unrestricted host access is added.
  • Vendor neutrality — existing adopter placeholders are preserved in skill and tool instructions; check-placeholders passes.
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — the new operation is read-only; it does not create tracker issues or send outbound messages.
  • Privacy LLM — private tracker issue bodies are saved in the controlled workspace rather than printed into model context; matching results expose only issue summaries.

Linked issues

Fixes #1730.

Notes for reviewers (optional)

Implementation: The new issue-import dedup-index command reads a saved tracker index and compares identifiers against the authoritative Security mailing list thread field. A dedicated issue-list-with-bodies vetted 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.

@KatalKavya96

Copy link
Copy Markdown
Contributor Author

@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:

  • prek run --all-files — all hooks passed.
  • Full workspace tests, Ruff, and mypy — passed.
  • Security-import Step 2 model evaluation — 4/4 passed.
  • Added regression coverage for backtick-wrapped Gmail IDs, archive identifiers, secondary threads, incomplete indexes, and caller authorization.

The implementation also fails closed when tracker completeness cannot be established, preventing unsafe duplicate imports.

Thanks!

@github-actions github-actions Bot added family:security security-* skills family:tools tools/* family:docs Docs, MISSION.md, READMEs capability:intake Import external signal + sync tracker state substrate:analytics Tool substrate: read-only metrics / dashboards / renderers substrate:sandbox Tool substrate: agent isolation, egress control, settings audit substrate:framework-dev Tool substrate: build / validate / eval the framework itself labels Oct 21, 2026

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Maildir ids. The maildir backend's file name goes through the same Gmail thread `<threadId>` template (outcomes/mailing-thread.md), but threads._THREAD_ID is [A-Za-z0-9_-]{6,} and stops at the first . — 1700000000.M1P2.host,U=12:2,S is never matched, though valid_ids accepts it.
  2. Sentinel first line. classify returns early on N/A — opened from public PR / N/A — imported from markdown file without reading the lines below, so a tracker issue-deduplicate merged from a PR import and a mail import loses its mail thread ids.
  3. Several roots. classify keeps only the first Root Message-ID; merge.py unions 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 calls dedup-queries / dedup-attribute after 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 potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@potiuk

potiuk commented Oct 30, 2026

Copy link
Copy Markdown
Member

@KatalKavya96, I merged main into this branch (a3cd55e). The conflicts were in tools/security-management/README.md, where I kept your dedup-index row and main's updated relay-prescreen row, and in the eval count: security-issue-import is now 43 cases across 9 steps. main now mirrors tools/security-management into plugins/magpie-security-tools/, so the merge also copies your dedup changes there. Please pull before pushing again. The two review threads are still open.


This comment was drafted by an AI-assisted tool and confirmed by an Apache Magpie maintainer.

@KatalKavya96

Copy link
Copy Markdown
Contributor Author

@potiuk I've pushed 0f84ac2 addressing both review threads and the obsolete-command cleanup. All local hooks are green, and I've replied to the inline comments. Thanks!

KatalKavya96 and others added 3 commits October 31, 2026 18:09
… 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 potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bare Gmail thread id. 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 rows main had moved on, including detect back to "sender pattern or preamble regex" (the code still requires both), 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. 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.

@potiuk
potiuk enabled auto-merge (squash) October 31, 2026 13:31
@potiuk
potiuk merged commit dc43391 into apache:main Oct 31, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:intake Import external signal + sync tracker state family:docs Docs, MISSION.md, READMEs family:security security-* skills family:tools tools/* substrate:analytics Tool substrate: read-only metrics / dashboards / renderers substrate:framework-dev Tool substrate: build / validate / eval the framework itself substrate:sandbox Tool substrate: agent isolation, egress control, settings audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security-issue-import: Step 2 thread-ID de-dup misses tracked threads (backtick-wrapped ids not found by search)

2 participants