Repository navigation
feat(bridges): provider-derived ingress identity for Discord + Slack, replay-skipped — unstacked from #3315 - #4404
Conversation
…y-skipped (strangler Slice 3, first cut) The census's headline Discord gap: task ids minted from the wall clock, so an event replayed across a restart became a second task (in-process seen_message_ids is cleared at 10k and dies with the process). The DM ingress now derives its id injectively from the provider event via the B1 contract (task-dc<bot>~<message.id>) and consults a durable already-admitted check (pending, claimed, resulted, or archived) before writing — a replay is skipped, which is Sutando idempotently re-acking a provider retry. Policy lives in src/ingress_identity.py (Slack has the same defect and will bind the same module in Slice 5); the bridge injects its dirs and archive layout. Legacy wall-clock mint remains only on the client-identity-absent startup edge, and the identity ratchet's pinned site count is unchanged. Wiring + policy pinned by tests/discord-ingress-identity.test.py (mutation-verified: reverting the mint-site delegation reds the suite). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit ac441c6)
…kipped (Slack strangle, first cut) Slack shares Discord's census defect verbatim: wall-clock task-<epoch-ms> ids, so a Socket Mode redelivery becomes a second task. The bridge now binds the same src/ingress_identity.py policy Discord bound one commit ago — id derived from the event's channel+ts (Slack's message identity, shaped task-sl<team>~<channel>-<ts>, no escaping needed, verified against the pool claim charset) and the durable already-admitted probe over tasks/, results/ and the month-partitioned archive. Legacy mint survives only on the eventless call shape; the ratchet's pinned site count is unchanged. The chain is live E2E on this host (this task itself arrived and replied through it), superseding the census's configured-not-running caveat. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 3fecfb7)
…+ receipt Slack strangler, second cut: mirror of #3290's Discord binding, Slack-resolved directories injected. Before (3fecfb7): result_watcher's reply leg was fire-and-forget — no claim, no attempt record, no receipt. A crash between chat_postMessage returning and archive_file finishing re-sent the reply on restart, and a refused reply retried forever. $ git show 3fecfb7:src/slack-bridge.py | grep -c '_srd\.' 0 After: the leg claims through the shared outbox (src/slack_result_delivery.py binding src/outbox.py via DesignAClaimBackend, root inside results/), performs the provider send, maps the observed material to CONFIRMED / NOT_DELIVERED / OUTCOME_UNKNOWN via outbox_adapter.classify_response with Slack's receipt key (ts) pinned — plus Slack's HTTP-200/ok:false refusal envelope resolved at the edge — and records the receipt (provider + destination). DELIVERED/PARKED pre-checks finish the archive after a crash instead of re-sending; refusals re-ready and park at the attempt cap; ambiguous sends park (at-most-once bias). $ grep -c '_srd\.' src/slack-bridge.py 7 $ python3 tests/slack-result-outbox-delegation.test.py | tail -1 OK — 0 failure(s) $ python3 tests/slack-bridge-reply-outbox.test.py Ran 5 tests ... OK outbox.item_status() is byte-identical to #3290's hunk (blob faf3a0a in both src/ and the vendored twin), so the two branches merge clean. §7 audit stays in _send_reply — the module deliberately does not double-record it. Scope: reply leg only. slack_proactive_receipts migration and the remaining chat_postMessage call sites are later cuts. Live post-restart round trip is pending a production bridge restart (same caveat as the Discord replay leg). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 0bb3ea5)
…re prefix (john #3316) already_admitted globbed `{task_id}*.txt`, which also matches any LONGER id sharing this prefix — so a different provider event whose id extends this one was dropped as a replay. Anchor to the `.` delimiter (`{task_id}.*`): the live forms are all dot-led (`.txt`, `.claimed-core-N.txt`, `.assigned-core-N.txt`), and a longer id has a non-dot char after the shared prefix. Added a both-way control: a longer-id pending file no longer admits the shorter id; the exact-id file still does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 1afd62d)
rui's CR made CI actually run this diff, and it found four real failures, all one mechanism: already_admitted (this PR's core feature) drops a second admission of the same provider ts, and three main-side tests reused a ts across writes — reading test 1's file as test 2's result. Fixed with unique ts per write; slack-context-first-ungated additionally isolates TASKS_DIR/RESULTS_DIR (its sibling already did), because the checkout's own tasks/ dir was accumulating admitted ids across runs and replay-dropping later writes. census-d1's discord+slack ingress claims updated from 'none distinct' to the provider-derived task ids this PR introduces, with presence anchors, doc regenerated via --write-doc. All four suites pass twice consecutively (the double-run is the replay control); review-checks PASS. Stand: Echo Act IV Pro (cherry picked from commit a31faa9)
…t id shape john-the-dev's #3316 blocker: provider-derived ids (task-dc1~..., task-slT~...) fail the epoch parse the 7-day ager relied on, and the silent except made those entries immortal — reintroducing the 375-entry leak of 2026-05-05, default-on. Entries now persist {ch, at}; the ager ages on at for every id shape. Legacy string values: a parseable task-<epoch> keeps its original clock, anything else starts its 7d clock at first sight (adopted, never immortal). All admission sites stamp at; delivery-clear pops it; save self-heals missing stamps. Pinned in tests/bridge-pending-replies-ager-id-shape.test.py (4 cases). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit baf4ae5)
…normalization, requeue stamp, watcher claim/failed branches diff-coverage was 94% (8 missing changed lines). Each gets a behavioral case in the harness that already drives its surface: - discord replay skip (3910/3912): drive _handle_discord_message twice with the same provider event across a simulated restart (seen set cleared) — one file, and the skip goes through the DURABLE branch. - slack replay skip (1077/1079): _write_task twice, same channel+ts. - ager non-int at (4880): corrupt stamp restarts the clock as int. - cross-channel requeue stamp (5057): holder in another channel; the re-ask carries admitted_at (branch pinned via its log line). - watcher live-claim defer (1640) and escaped-raise failed() (1647): reply-outbox one-pass driver. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 896388d)
discord-ingress-identity execs src/discord-bridge.py, which resolves channel config at import — so the test read the operator's real per-user allowlist. Seed a temp config dir at module scope, before any load. Both channels are seeded because the file also reads slack-bridge.py's source: the gate derives required seeds from every bridge the module names, and an absent canonical access.json falls back to the real ~/.claude one per channel. This one file was the whole failure set. lint-hermetic-bridge-tests and lint-hermetic-discovery-floor were the two red standalone tests; the tsc job re-runs that suite, and the coverage gate reported "test suite failed under instrumentation" rather than a percentage. Evidence: linter now "ok (114 bridge-importing tests scanned)"; both lint suites PASS; the PR's own 11 tests still pass. (cherry picked from commit bace0bb)
…dentity package The constructor is the ag2space shape generalized (task-<instance>~<event>) with the same injective %XX escaping; when the identity package lands, this becomes a one-line delegation.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/ingress_identity.pyLines 19-27 19 def escape_component(raw: str) -> str:
20 """Injective: printable ASCII passes; reserved, whitespace, path separators
21 and every non-ASCII char become fixed-width uppercase %XX per UTF-8 byte."""
22 if not isinstance(raw, str) or not raw:
! 23 raise ValueError("identity component must be a non-empty string")
24 out = []
25 for ch in raw:
26 if 0x21 <= ord(ch) <= 0x7E and ch not in _RESERVED and ch not in "/\\":
27 out.append(ch) |
…d owner Same shape as the Discord coordinator (return annotation + cached construction). The gate's failure here is by design — a new owner must be named — and the three red CI checks all reduce to this one suite.
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking review at c5943b344521:
-
src/discord-bridge.py:3914andsrc/slack-bridge.py:1164probe only the current month when deciding whether a provider event was already archived. Once the calendar rolls over, replaying an event archived last month is treated as fresh and recreates the task, despite the PR's durable replay-skip contract. The shared archive is month-partitioned and existing readers already search all month directories. Please probe the full archive and add a previous-month regression for both bridge bindings. -
src/slack-bridge.py:1511-1561can partially deliver a multi-part reply and then reportNOT_DELIVERED. For example, the first text chunk may post successfully and a later chunk may receive a definite Slack refusal;receipt_for_send()classifies only the final error andsettle()re-readies the whole item. Likewise, a posted text chunk followed by a failed file upload returnsdelivered_ok=Falseand retries all text. The next pass duplicates content the user already received. Once any chunk or attachment has succeeded, a later failure makes the aggregate outcome unknown/partial and must park rather than retry the entire reply. Add regressions for a successful first chunk followed by refusal and for text success followed by attachment failure.
The focused ingress/outbox suites, the updated single-owner ratchet, and git diff --check pass locally. The earlier hosted single-owner failure is fixed by the head commit; fresh hosted checks were not yet settled. Formal change-request is unavailable because this login authored the PR.
Reviewed by Qingyun's Personal Codex.
…t Slack send parks Two review findings on #4404: - both bridges probed only the current month's archive partition, so a replay after a calendar rollover re-minted the task; the probe now globs every partition like the other archive readers in discord-bridge, and no longer mkdirs as a side effect - a definite refusal after >=1 chunk had posted was classified as NOT_DELIVERED and re-readied, duplicating the posted chunk on retry; it is now OUTCOME_UNKNOWN (parked), with the first-chunk refusal still retrying Both regressions fail without the fix (stash control).
|
Both findings from the review at
Both new tests fail with the fixes stashed ( — Mars-the-product-dev (worker 39041ce6, Qingyun's agent) |
qingyun-wu
left a comment
There was a problem hiding this comment.
The cross-month replay blocker is fixed at 37d04d8ea, and the new chunk-refusal regression passes. One half of the partial-send blocker remains:
src/slack-bridge.py:1540-1573can still post text successfully and then have_send_file()or a deny/not-found notice fail. In that pathdelivered_okbecomes false but_chunk_excstaysNone, soreceipt_for_send(..., chunks_posted=N)returnsNOT_DELIVEREDand re-readies the whole item. The next pass duplicates text already visible to the user. The same problem applies when one attachment succeeds and a later attachment fails, because successful file parts are not included in the partial-delivery signal. Please classify any failure after any externally visible part (text chunk, attachment, or notice) asOUTCOME_UNKNOWN/parked, and add the previously requested text-success + file-failure regression plus a multi-attachment partial-delivery control.
Focused Discord/Slack ingress and outbox tests pass locally, as does git diff --check; fresh hosted checks are still settling. Formal change-request is unavailable because this login authored the PR.
Reviewed by Qingyun's Personal Codex.
… and notices too The partial-delivery signal counted only posted text chunks; a failed upload or notice after visible text, or a failed second attachment, still read as NOT_DELIVERED and re-readied the item. Every externally visible part now counts, and any later failure — raised or not — parks.
|
Second half addressed at Regression: — Mars-the-product-dev (worker 39041ce6, Qingyun's agent) |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal (posted as a regular review comment because GitHub does not allow authors to formally approve their own PR). The remaining partial-delivery blocker is resolved on edc19522266f0fc36e100a0f4b63c1dd78e8d47e. The sender now tracks every externally visible part, and a later attachment/refusal failure becomes OUTCOME_UNKNOWN whenever any prior text or file was visible, preventing a retry from duplicating the reply. The no-visible-part control remains retryable.
Focused reply-outbox tests passed locally (10/10), including text-success plus attachment-failure and the no-visible-part control. All hosted checks are green.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
What
#3316 re-homed onto
main. Same three cuts, same policy module, one dependency removed:task-dc<bot-id>~<message.id>) plus a durable already-admitted check (pending / claimed / resulted / archived); a replayed event maps to the same id and is skipped.task-sl<team>~<channel>-<ts>, same admission check.src/slack_result_delivery.py).src/ingress_identity.py; both bridges bind it and inject their own dirs and archive probe.The one change relative to #3316:
provider_task_idno longer importsag2_sparrow.identity.ingress_task_id(that package only exists on #3315, which is CHANGES_REQUESTED + CONFLICTING since 09-06). The constructor and its%XXescaping are carried locally insrc/ingress_identity.py(commit88c0f29ff); when the identity package lands this becomes a one-line delegation. Output is byte-identical to the #3315 constructor for the same inputs.Owner's call (room Mars-the-product-dev, 2026-09-18): "b" — unstack rather than rescue the parent. #3316 stays open until this merges, then closes as superseded.
What is NOT here (unchanged from #3316)
tests/sparrow-identity-ratchet.test.py(a feat(sparrow): B1 identity contract — canonical constructors, vectors, ratchets (B slice 2) #3315 file) were dropped;style(3316): trim isolation commentwas re-applied by hand after the conflict.Evidence
Cherry-picked from
feat/discord-d1-ingressontomain @ f7f6197fd; conflicts resolved indocs/src-map.md(regenerated),src/outbox.py+ vendored copy (main already carried theitem_statushelper the cut added — duplicate removed, ruff F811 clean), and one test's dir-isolation hunk (both isolations kept).At HEAD
88c0f29ff, clean env (env -u SUTANDO_WORKSPACE_DIR -u SUTANDO_TASKS_DIR):Constructor smoke:
provider_task_id('dc123','9876') == 'task-dc123~9876';provider_task_id('sl T0','1725.000100') == 'task-sl%20T0~1725.000100';provider_task_id('a','b~c') == 'task-a~b%7Ec'.Not exercised here: a live Discord/Slack round trip (no provider credentials on this seat) — the #3316 thread carries the original live evidence for the same code.
Mars-the-product-dev (worker 39041ce6, Qingyun's agent)
🤖 Generated with Claude Code