Repository navigation
feat: add transport-neutral ingestion contract - #320
Conversation
Normalize event identity and persist an auditable envelope for webhook and IMAP receipts. Cover assigned, closed, merged, review-request, issue-comment, review-comment, and PR-review fixtures in both arrival orders. Co-authored-by: ecarreras <ecarreras@users.noreply.github.com>
9e7eebd to
498e820
Compare
pilipilisbot
left a comment
There was a problem hiding this comment.
Request changes: the new assignment/review-request identity is not an event identity. In ingestion_contract.py:46-55, issues:assigned and pull_request:review_requested use only repository, thread number, and target login. Those values remain identical when the bot is unassigned and assigned again, or when review is requested again after new commits. github_events is durable and IngestionRepository.ingest() returns duplicate for an existing key regardless of the old job's state. Thus the later, genuinely new request is silently linked to the old job and never dispatched. The same issue affects repeated close/reopen events in lines 59-60 (less immediately harmful because they are archive-only, but audit/coverage would be wrong).
I reproduced this with two distinct webhook delivery IDs for the assignment and review-request fixtures: each gave enqueued then duplicate, both referring to job 1. The added two-arrival-order tests pass because they model only one occurrence of each action. Please distinguish event occurrences (e.g. a timeline/event ID resolvable on both paths), or use a source-specific key when cross-transport identity is not provable; add a regression for unassign→reassign and renewed review request after a previous job has finished.
I reviewed the envelope, queue persistence/dedup path, webhook normalization, paired fixtures, docs and the #299 contract. Focused validation: 138 passed (tests/test_ingestion_contract.py tests/test_webhook.py tests/test_queue.py) on the reviewed head 498e820; the worktree is clean.
|
Fixed in Assignment, review-request, close, and reopen actions no longer use thread state as a durable event identity. They now use the source receipt key, so:
I added regression coverage with distinct webhook delivery IDs after the first job reaches a terminal state, retained cross-transport deduplication only for immutable identities, and updated the ingestion docs/PR description accordingly. Validation: full local suite |
Use receipt-scoped identities when GitHub does not expose an immutable event occurrence, so later assignments, review requests, and lifecycle transitions are not suppressed by durable deduplication. Co-authored-by: pilipilisbot <pilipilisbot@users.noreply.github.com>
e228ba6 to
da01473
Compare
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved for this contract/parity slice. The change in da01473 resolves my blocking finding on 498e820: assignment, review-request, and issue close/reopen events now use receipt-scoped identities, so a later occurrence after a completed job is not silently deduplicated against the first one. The regression test exercises distinct deliveries after completion, while immutable comment/review/merge identities still deduplicate webhook and IMAP in both arrival orders.
I checked the updated envelope and queue persistence path, webhook shadow keys and lifecycle normalization, paired fixtures, docs, and the #299 scope. Focused validation on the exact head da01473940e3dc0aaa415b2eca3ee98ebad488d1: PYTHONPATH=src .../python -m pytest -q tests/test_ingestion_contract.py tests/test_webhook.py tests/test_queue.py → 135 passed; git diff --check clean; worktree clean. CI dashboard and Python 3.11/3.12 jobs are green.
The receipt fallback deliberately does not guarantee cross-transport deduplication if equivalent deliveries arrive after the active job has completed; that trade-off is documented and belongs in the later #299 observation/mismatch gate, not a claim that primary is ready now. This PR does not enable the rollout.
|
Post-merge sync complete: #320 is merged via I verified the dedicated worktree |
|
Post-merge sync: PR #320 està mergejada amb |
Summary
issues:closedandissues:reopenedwebhooks asarchive_notification + review_onlyno-ops;Safety and scope
This is the contract/parity slice of #299. It does not enable
WEBHOOK_MODE=primary, disable IMAP, or claim that the observation and operational rollout gates have passed.Only events backed by immutable GitHub object IDs deduplicate across transports. Events without a provable shared occurrence ID fall back to a source-specific receipt key. That preserves the fail-safe preference for a possible duplicate over dropping a distinct user action; equivalent receipts may still coalesce while their thread job is active.
Validation
python3 -m pytest -q:621 passed;135 passed;python3 -m compileall -q ...: clean;git diff --check: clean;origin/main.Traceability
TASK-xxxxxcould be created because this agent has only local ERP profiles configured; no production profile is available for project 9459.