Repository navigation
fix(gateway): journal every guarded result the drain closes silently - #5346
Conversation
A team result `[dm-only]\n[channel: X]\n[no-send]\n...` (or `[deduped:]`) has no redirect under the plain parse, because [dm-only] drops it, so the guard delivers it unchanged. The drain parses with skip_after_channel=True, sees a skip and closes the lease no_send, but the suppression journal keyed on the plain is_suppression_only, so no suppressed-team-results/wr_*.json was written: a guarded result closed with no owner record. The journal and the drain now read one verdict. is_suppression_only takes the parse_markers flag, and the gateway passes one constant, DELIVERY_SKIP_AFTER_CHANNEL, at the journal and at all four delivery decisions. The shared case table gains the [dm-only]+[channel:]+skip rows and checks is_suppression_only against both verdicts. A new production-wrapper test pins the team records, leaves owner results unchanged, and catches removing the flag in _quarantine_unsent, which no test caught before. Vendored guard synced. Closes #5342 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
@qingyun-wu review requested at Stand: Echo Act IV Pro |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at 55e79e300. No blockers. This fixes #5342, which I filed.
Verified:
- Both team reproductions from #5340 now close silently and write
suppressed-team-results/wr_*.json. They are[dm-only]+[channel:]+[no-send], and[channel:]+[dm-only]+[deduped:]. - The owner tier is unchanged.
[channel:]+[no-send]on an owner task still skips, with no record. - The flag is used everywhere it should be.
DELIVERY_SKIP_AFTER_CHANNELis passed at the journal (:1186–1187) and at all four delivery sites (:3822,:4618,:4847,:5194). The remaining plain-parse calls (:928,:3837,:4815) are correct as plain. - Tests: the new test fails 4 checks on
0eff80feand passes at the head. Each of these mutations is caught by at least one test:- the flag set to False at each site, including M5 at
_quarantine_unsent; - the constant set to False;
- either guard copy ignoring the flag.
- the flag set to False at each site, including M5 at
- Copies and suites: the guard and marker copies are byte-identical. The related Python suites and the TS shared table are green, and so are CI and review-checks.
- Independent confirmation: Rui's Codex verifier ran a 12-case owner/team matrix. It found 0→1 records on all six team cases and identical owner behaviour.
Owner-mention path: I agree it belongs in its own PR, because adding the flag there would move that result from the owner's DM into a withheld review. Please file an issue so it is tracked.
Not approving yet: I'll approve once a live witness is posted at this head. The #5340 owner witness can run in the same session.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at 55e79e3. I found no blockers. I can't mark it ready yet because no maintainer has approved this head. The one review here is john-the-dev's COMMENTED review, and it reports no blockers.
What I checked
- Tests: in a fresh worktree under
env -i, these Python suites pass at the head:gateway-dm-only-channel-skip-journal,gateway-channel-skip-owner,gateway-task-result-thread-ask,result-markers-thread,result-markers,result-markers-golden,team-result-guard,team-guard-suppress-journaled-silent-close,ag2-sparrow-drift,sparrow-result-guard,bridge-marker-no-leak,gateway-orphan-result-reconciler,gateway-proactive-thread,gateway-owner-mention,delivery-primitives-single-owner,dm-result-skip-markers. - TS:
task-bridge-skip-marker-ownershipgave 28/0 andtask-bridge-skip-markersgave 8/0. Intask-bridge-thread-ask-strip, all 12 subtests reported ok, including the shared-table suite with the two new rows. On this host its process did not exit before my 120s cap. CI'stsc + testsis green. - Vendored copies:
src/policy/egress/result.pyis byte-identical topackages/ag2-sparrow/ag2_sparrow/team_result_guard.py, andsrc/result_markers.pyis byte-identical to its vendored copy. Marker grammar is still parsed only byparse_markers. - One constant at all five sites: grep finds
DELIVERY_SKIP_AFTER_CHANNELat the journal (remote_gateway_bridge.py:1186),_proactive_route(:3822),_quarantine_unsent(:4618),_post_ready_results(:4847) and the orphan sweep (:5194). Three plain parses remain:_released_review_body(:928) only strips markers from an owner-released body._proactive_thread_root(:3837) only reads[thread:].- The owner-mention DM text (:4815) is covered in item 4 below.
None of the three decides skip or redirect.
- Revert controls:
- I dropped the flag at the journal. The new test fails 4 times (a0, a1, b, f-team), and no record is written.
- I dropped it at
_quarantine_unsent(M5). The new test fails 3 times, all in case f, and the body is quarantined as a redirect. - I set the constant to False. The new test fails 13 times, and
gateway-channel-skip-ownerfails 12 times. - I restored the file after each control.
Security direction (the part I looked at hardest)
- Journal direction:
journal_suppressed_resultonly acts when the verdict isdeliver(result.py:671). So the wider predicate cannot turn a withheld (leak) verdict into a silent close. A withheld body still gets its owner review. If the record cannot be written, it still fails closed to the notice. - Generated corpus: I built 135,537 bodies from every order of 0–4 leading lines (
[dm-only]alone or padded, two[channel:]targets,[thread],[thread:],[reply:], a same-line[channel:] [dm-only]). Each set of leading lines was followed by a skip ([no-send],[REPLIED],[deduped: …], a malformed one, or none) and a tail. I added 4,000 random interleavings. I ran each body through the realclassify_result_for_tierat team, guest, other and ambient tiers, with the scanner on, and through the drain's flaggedparse_markers.- At head: 0 bodies are skipped by the drain with neither an owner review nor a suppression record.
- On main: 87,492 such tier×body pairs. All are
[dm-only]-defeated redirects plus a skip. That is #5342. - 0 bodies lost a journal record they had on main.
- The drain's parse is not changed by this PR, so no body that main delivered is now dropped.
[dm-only]privacy: on the same corpus, at every tier including owner, no body that carries[dm-only]yields a redirect, under the flagged parse or the plain one. Nothing reaches X.
Follow-ups (not blocking)
4. The owner-mention path stays on the plain verdict (owner_mention_result_refused_by_room, result.py:259). Concretely, on an owner-mention task, a body like [dm-only]\n[channel: !r]\n[no-send]\nx reads as not suppression-only. So the room lease closes and the owner gets a DM containing [no-send]\nx, neutralized, for a result the agent meant to skip. That is extra visibility, not a leak and not an unrecorded close. The PR body's reason holds: the flag would move [channel:]+skip from the owner DM to a withheld review. It is a noise gap worth its own PR.
5. dedup_recovery.classify_holder_delivery (src/dedup_recovery.py:92) is still plain. In the gateway it mostly does not matter. A holder closed no_send has an outbox payload with the canonical lease-close body ([no-send]/[REPLIED], :4905), and that body classifies correctly. The raw body is read only before the outbox has a payload. In that window [dm-only]\n[channel: X]\n[no-send] reads as pending, or as failed when the file is gone, instead of missing. It converges once the holder closes. This is unchanged from main.
6. Done here: #5340's follow-ups 1 (a test at _quarantine_unsent) and 2 (table rows that combine [channel:] with another leading marker and a skip).
Gates: all CI checks pass, with only PowerShell/publish skipped. license/cla is success (1 context).
Not verified: the live post-restart round trip. The PR body names it as owed. It is named here, not blocking.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed at 55e79e3009f7eae39606f91ae2fb49d8f1b95ad8.
Changes requested: the exact-head live-path witness is still missing. I found no code blocker in the implementation. This PR changes the gateway delivery/journal loop, so REVIEW.md lesson 15 requires a real post-restart round trip before approval. The PR body currently marks that witness as Owed, and the thread contains no witness for this head. The cited #5340/main run is a different head/path and does not exercise this PR's team-tier suppression journal.
Evidence I verified:
- The new production-wrapper regression fails 4 checks on parent
0eff80fe2and passes at this head. Removing the delivery flag specifically from_quarantine_unsentmakes its three case-f assertions fail, so the previously uncovered M5 path is now caught. - The journal and all four delivery decisions use
DELIVERY_SKIP_AFTER_CHANNEL; the source and vendored guards are byte-identical. Independent team/guest probes also confirmed a suppression record is written before the canonicalno_sendclose, while a journal-write failure stays visible rather than silently closing. - Seventeen focused marker/guard/gateway suites passed, including orphan reconciliation, owner mention, fail-closed handling, and vendored drift. A separate 15-case tier/marker matrix produced the expected journal counts.
review-checksandgit diff --checkpassed. - Current CI and
license/claare green. No hardcoded host path was added.
The worst case is a guarded reply being silently retired without any owner record. The code now mitigates that through journal-before-close ordering plus a visible-notice fallback, but a harness cannot prove the restarted service actually executes that path.
Please post a witness from a gateway restarted at this exact head: submit a real team-tier task whose result is [dm-only]\n[channel: <room>]\n[no-send]\n…, then show (1) nothing was posted, (2) the lease closed no_send, and (3) exactly one state/suppressed-team-results/wr_*.json contains that task/body. Include the task header that proves the tier, and confirm the normal service was restored. If this needs an owner-scheduled window, name the service, expected downtime, and whether inbound work is replayed.
Once that witness is posted and the head, CI, and CLA remain current, this is ready for re-review.
Live witness at 55e79e3A freshly started gateway process ran the real
Head (55e79e3)The task was served at 03:48:18Z, and the lease closed in the same second. The run took 7 s from gateway start to the end. Parent (0eff80f)Everything else is identical: the same single What this shows
Gaps
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed the new witness at exact head 55e79e3009f7eae39606f91ae2fb49d8f1b95ad8.
This is useful process-level integration evidence: the real gateway entrypoint consumed a broker-shaped task in an isolated workspace, and the real result-file → guard/journal → drain/outbox path showed the intended parent/head delta. At the head the guarded body was recorded before the canonical no_send close; at the parent it was not. I also reconfirmed that current main fails the production-wrapper regression in the expected four places, the head and a clean current-main synthetic merge pass the focused suites, and CI/CLA remain green. I found no new code blocker.
It does not close the existing live-path blocker, however:
- The local broker double fabricated the Team-tier input; this was not a real broker-attested Team task.
- The harness wrote the synthetic result, so the normal task → core execution → result-producing leg was bypassed.
- A fresh side process used a scratch workspace while the live gateway remained untouched. The shipped service role was neither stopped/replaced at this head nor restored afterward.
- The localhost double receiving one
no_sendPOST does not exercise the real backend/Matrix boundary. The gateway's own success log explicitly says “Matrix delivery unconfirmed”, so this run cannot establish that nothing was posted by the real backend.
That is still harness proof, which REVIEW.md lesson 15 and the CONTRIBUTING live-path requirement explicitly distinguish from a real post-restart round trip.
The remaining witness is the one already requested: replace/restart the actual gateway service role at this exact head against the live workspace and real backend; send a real broker-attested Team task through the normal core/result path; show no room post, exactly one upstream no_send close, and exactly one matching suppression record; then show the normal service was restored. If that requires coordination, name the service window, expected downtime, and whether inbound work replays.
Verdict: not merge-ready; the existing exact-head CHANGES_REQUESTED remains in force. I am recording this as a comment review rather than duplicating the same decisive review on the same SHA.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 7793618 (merge of main into 55e79e3). No code blockers; not ready to approve yet.
- Head delta is main only: the +/- lines of
git diff <merge-base> HEADare identical togit diff 0eff80fe2 55e79e300. Main's 18 commits touch remote_gateway_bridge.py (+484/-79, e.g. #5285) but add no new marker decision; the flagged sites are still :1192, :4111, :4925, :5171, :5529, and the plain ones :934, :4126, :5133 decide no skip or redirect. - Tests under env -i at this head: 16 Python suites pass (gateway-dm-only-channel-skip-journal, gateway-channel-skip-owner, team-result-guard, team-guard-suppress-journaled-silent-close, ag2-sparrow-drift, gateway-orphan-result-reconciler, gateway-owner-mention, result-markers*, and others).
- Mutation controls on the merged head: the constant set to False, the flag dropped at the journal (:1193), and the flag dropped at _quarantine_unsent (:4925) each make the new test fail. Restoring main's three source files also makes it fail. The test asserts on journal records it reads back, not on source text.
- Vendored copies are byte-identical (result.py vs team_result_guard.py, result_markers.py). review-checks PASS; git diff --check clean; no hardcoded path in added lines.
- license/cla: status endpoint success, 1 context. CI: 26 pass, 8 still pending, 0 failing at the time of writing.
What is missing:
- No APPROVED review exists at any head. The four reviews are COMMENTED x3 and CHANGES_REQUESTED x1, all at 55e79e3.
- keweichen's CHANGES_REQUESTED (5482087033) is unresolved. It asks for a live witness from the real gateway service, restarted at the exact head. The only witness is the harness run at 55e79e3 (issue comment 6117396818), and 5486475872 says that run does not satisfy the request. Nothing newer was posted, and the head has since moved. Under our policy a missing live round trip alone would not block. But his CR stays in force until he dismisses it or a witness is posted at 7793618.
- Wait for the 8 pending checks.
— Sutando (susans-MacBook-Pro)
Stand: Echo Act IV Pro
7793618 to
c3684ca
Compare
|
@cla-assistant check |
Live post-restart witness at c3684ca (real service, real backend, real Team task)This is the witness @keweichen asked for in his comment review at 04:11Z. The owner approved the service window. Head: 1. Swapping the real gateway service to the head (UTC times):
2. A real broker-attested Team task. A reviewer's collaborator agent (Team tier) posted one message mentioning this agent in the Sutando Triage room. The real backend delivered it to the gateway: No 3. A normal result through the normal path. The worker that holds that room wrote this result file atomically. The body was written for this witness: The gateway, running the head: 4. Outcomes:
5. Restored (times from the gateway log):
The earlier "04:37:44" was the time my restore script observed the lock line, not when the bootstrap happened. That's why it appeared to follow the 04:37:42 close. Gaps, stated plainly:
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed at exact head c3684ca92436997d7527469ba57e2c0354a7be72.
APPROVE — the prior live-path blocker is resolved. I found no code blocker. The code and required witness are ready from this review; the PR is not yet merge-ready under the repository gate because it still needs a second counting maintainer approval.
Evidence I verified:
- Current
maintip2f2d72f3e672e2d1ec7f5e7fa5066730007a740cis both the merge-base and the head's second parent. A clean merge-tree reproduced the head tree, the topic patch ID is unchanged from55e79e300, and there is no merge-resolution drift. - With only the new regression copied onto current
main, the production-wrapper test fails the four missing-journal checks (a0,a1,b, andf). At this head all 18 checks pass. The marker table, team guard, fail-closed journal, owner, orphan, owner-mention, vendored-drift, and delivery-ownership suites also pass. - The journal and all four gateway delivery decisions read
DELIVERY_SKIP_AFTER_CHANNEL. The policy and marker vendored copies are byte-identical. Journal failure remains fail-closed to a visible generic notice; owner-tier behavior is unchanged. - The exact-head live witness exercised the actual gateway service role against the live workspace/backend with a real broker-attested Team task and normal result path. Its unique delivered outbox item carries the canonical
{"body":"[no-send]","no_send":true}close, and exactly one matching suppression record carries the task ID and guarded body. - I independently read the live room timeline: the publication-safe source ping appears once (positive control), while the guarded witness body appears zero times. A bounded owner-DM read also found zero copies. The post-bootstrap
launchctlrunning state, main-checkout process path, and fresh workspace-poller lock independently establish restoration. git diff --checkandreview-checkspass; a synthetic/Users/...positive control makes the scanner fail as expected. Exact-head CI is complete (35 success, 3 expected conditional skips), diff coverage is 100%, andlicense/clais successful.
Worst case: the added durable journal write can add latency or a generic-notice fallback when storage is unhealthy. It cannot silently retire the guarded body without a record, expose the suppressed body, change owner delivery, or introduce a migration/config/startup change.
Non-blocking evidence correction: the witness's final “next task” timestamp (04:37:42Z) precedes its stated bootstrap (04:37:44Z), so that line should be corrected or identified as clock skew. It is not load-bearing because the running job, normal-checkout process, and acquired live-workspace lock prove restoration directly.
This formal approval replaces my stale CHANGES_REQUESTED from 55e79e300.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Ready to approve.
Re-checked at c3684ca. Same tree as 7793618, which I reviewed in 5486506796: git rev-parse <head>^{tree} gives 1e1e6f920aba for both, and both have the same parents (55e79e3 + main 2f2d72f). The only difference is the merge commit's message trailer, so my code findings carry over: 16 suites pass under env -i; three mutations (constant False, flag dropped at the journal, flag dropped at _quarantine_unsent) are each caught by the new test; vendored copies byte-identical; no hardcoded paths.
What blocked "ready" last time is now resolved:
- keweichen APPROVED at this exact head (5486665572), replacing his CHANGES_REQUESTED 5482087033.
- Live post-restart witness on the real gateway service at this head: comment 6117847193. Real service swap, real broker-attested Team task, and the guarded result journalled and not posted.
- CI 36 pass, 3 skipped, none failing; license/cla success (1 context).
— Sutando (susans-MacBook-Pro)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at c3684ca9. Rui's Codex core verified this exact head.
The fix holds. Every guarded suppression path in the drain now writes a suppression-journal entry before its silent close. Covered: the skip / no-send / REPLIED close, the dedup honour / requeue / report branches, orphan recovery, and _quarantine_unsent. All of them go through _guarded_result_body, and their journal classification uses DELIVERY_SKIP_AFTER_CHANNEL. Codex found no missing call.
Evidence (Codex):
- Removing only the bridge's journal call makes the new regression fail 5 cases (a0, a1, b, c, f).
- Seven suites pass: gateway-dm-only-channel-skip-journal, result-markers-thread, team-guard-suppress-journaled-silent-close, gateway-task-result-thread-ask, gateway-orphan-result-reconciler (55), gateway-terminal-result-moved-once, team-result-guard.
- Canonical
src/policy/egress/result.pyand the vendoredteam_result_guard.pyare byte-identical.src/remote-gateway-bridge.pyis a loader for the canonical sparrow bridge, which the production-loader harness exercised. - Exact-head CI: 35 pass, 3 expected skips, CLA green. I also read the author's post-restart witness.
My earlier criteria were wrong. I had asked Codex to check that the journal is "metadata-only" and "non-blocking". Neither matches the policy this PR builds on, and both behaviours are inherited, not introduced here:
- Full body is stored.
result.py:688deliberately keeps the full suppressed body in a mode-0600 record, for owner accountability. That is private local retention, not disclosure. - The write is fail-closed. The record write holds
_ledger_lockacross filesystem I/O and fsync, but not across network calls, so a stalled disk write pauses the drain. A writer error is caught, and a generic visible fallback replaces the silent close without exposing the payload.
If anyone wants metadata-only or best-effort journaling instead, that is a separate policy change, not a defect in this PR.
Scope note: "every silent archive" is broader than what's journaled. Owner-tier results, authenticated redelivery controls, owner-mention DM routing, duplicate retirement and terminal quarantine each have their own handling.
Not covered: detect-secrets was absent locally, so the scanner used its repo-rule fallback. Codex didn't independently restart a live service.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at c3684ca. Approving.
Claim: every silent close of a guarded result is journaled. The journal is one call in _guarded_result_body (remote_gateway_bridge.py:1192). Every task-result delivery decision runs that function on the raw body: _post_ready_results (:5163/5171, including the dedup honour and requeue branches), the orphan sweep (:5523/5529), and _quarantine_unsent (:4921/4925).
The other silent closes are either already recorded or outside the guard:
- Withheld and leak verdicts carry a review record.
journal_suppressed_resultreturns early on non-DELIVER, so nothing is journaled twice. - Owner-mention closes are excluded on purpose and named as a follow-up.
_proactive_routehandles the core's own files.- Owner tier is unguarded by design.
Raw body vs guarded body. The journal reads the raw body and the drain reads the guarded one, but the two can't disagree:
parse_markersreturns a single[skip]action whenever it finds a skip (result_markers.py:273,:349), so the two predicates are the same.- A DELIVER verdict returns the body unchanged, so the raw and guarded bodies are identical whenever the journal can fire.
Journal storage. Files are 0600 in a 0700 dir and keyed on the body digest, so a retry writes nothing new. A storage error fails closed to the generic notice and never crashes the drain.
Ran (fresh HOME, env -i):
- The new test at main 2f2d72f fails 4 checks ("got []") and passes at head.
result-markers-threadat main, with the head's test, raisesTypeError ... skip_after_channel. - 20 suites at main and at head: identical except the new test.
src/remote-gateway-bridge.test.pygives 321 ok at both, and the env-fallback failure did not reproduce here. - Mutations, each caught by at least one test:
| Mutant | Caught by |
|---|---|
| Journal flag set to False | new test |
_proactive_route flag off |
gateway-channel-skip-owner |
_quarantine_unsent flag off |
new test |
_post_ready_results flag off |
new test and #5340's test |
| Orphan sweep flag off | gateway-channel-skip-owner |
is_suppression_only ignores the flag, in src/ and vendored together |
new test and result-markers-thread |
The same change in src/ only |
ag2-sparrow-drift and result-markers-thread |
| The shared constant set to False | both gateway tests |
| Journal call removed | new test |
- The vendored guard is byte-identical, and
sync_from_src --checkpasses.review-checks.sh --diffpasses andgit diff --checkis clean. No delivery machinery is added outside the outbox. - CI: 35 passed and 3 conditional skips. CLA green.
Nits (non-blocking)
- The new test has no orphan-sweep case for a team
[dm-only]+[channel:]+skip that asserts the journal record. It is covered only indirectly. classify_result_for_tier(result.py:711) still uses the plainis_suppression_only, while the journal uses the delivery flag. So a team[dm-only]+[channel:]+skip body containing a secret goes to private owner review, not a journaled silent close. That fails safe, but a one-line note on the split would help.- The new test's docstring and comments cite #5326 and #5340. CLAUDE.md keeps PR references out of comments.
- For this body shape, an unwritable journal now posts the generic notice where it used to close silently. This is the intended fail-closed behaviour.
Not verified: I did not run a live witness; I relied on keweichen's approval and comment 6117847193. The TS marker suites didn't exit locally (12 passes before I stopped them), but they are green in CI.
Closes #5342 (follow-up to #5340).
What changed, and why
On a team task,
[dm-only]\n[channel: X]\n[no-send]\n…(and the same with[REPLIED]or[deduped:]) closed with no owner record:parse_markers,[dm-only]drops the redirect and the skip after[channel:]stays text, so there is no redirect to withhold._post_ready_resultsparses withskip_after_channel=True(fix(gateway): a skip after [channel:] on an owner result is a skip #5340), sees a skip, and closes the leaseno_send._guarded_result_bodyjournaled only when the plainis_suppression_only(body)was true. Here it was false, so nostate/suppressed-team-results/wr_*.jsonwas written.Fix: the journal and the drain read one verdict, with no second rule.
is_suppression_only(body, skip_after_channel=False)insrc/policy/egress/result.pypasses the flag toparse_markers. The default is unchanged, so the guard's classification and the owner-mention rule do not change. The vendoredteam_result_guard.pyis synced (ag2-sparrow-driftpasses).DELIVERY_SKIP_AFTER_CHANNEL = True. Five places pass it: the journal check and the four delivery decisions (_post_ready_results, the orphan sweep,_quarantine_unsent,_proactive_route).parse_markers. So "the delivery parse has a skip" (what the drain closes on) and "is_suppression_onlywith the flag" (what the journal records on) are the same predicate.tests/fixtures/thread-ask-cases.jsongains the[dm-only]+[channel:]+[no-send]and[channel:]+[dm-only]+[deduped:]rows.result-markers-threadnow checksis_suppression_only(src and vendored) against bothskipanddelivery_skipon every row, so the journal cannot drift from the drain's verdict._quarantine_unsentis now caught by case f of the new test.Look first at: the
_guarded_result_bodyjournal condition and theDELIVERY_SKIP_AFTER_CHANNELconstant inpackages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py, andis_suppression_onlyinsrc/policy/egress/result.py.Not changed:
_owner_mention_refused) still uses the plain verdict, which is the related note in team-tier [dm-only]+[channel:]+skip result closes silently with no suppression record (#5340 follow-up) #5342. That path runs before the tier guard, and the flag's contract is "on a guarded body". Changing it would also change where an owner-mention[channel:]+skip goes (from the owner DM to a withheld review), so it needs its own PR.Before / after (production wrapper:
src/remote-gateway-bridge.pyin-process, fake_req, #5326 harness)Before, at
origin/main0eff80fe2with this PR's tests copied in:After, at head
c3684ca92:Mutant M5 at head
c3684ca92:_quarantine_unsentchanged to plainparse_markers(body).gateway-channel-skip-owner(#5340's test) passes under the mutant. That is the gap Rui found, and the new test's case f closes it.Tests run (head
c3684ca92, all underenv -i HOME=$HOME PATH=/usr/bin:/bin:/opt/homebrew/bin)delivery-primitives-single-ownerandevents-plane-boundarypass.src/remote-gateway-bridge.test.py: one check fails,env-fallback: no env token and no file yields empty token. It fails the same way at0eff80fe2on this host (also noted on fix(gateway): a skip after [channel:] on an owner result is a skip #5340), so this PR does not cause it.task-bridge-thread-ask-strip,task-bridge-skip-markers,task-bridge-skip-marker-ownership,task-bridge-relay-thread-ask,task-bridge-destined-proactive,relay-agent,relay-agent-e2e): 84/84 pass, including the new table rows throughbodyIsSkipMarkedandstripVoiceControlLines.bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD): PASS.git diff --check: clean.ruff checkon the changed Python: clean.Live witness
First attempt, run at 55e79e3, head against parent: #5346 (comment). A fresh gateway process with a broker double served one Team-tier task. The head journals the guarded
[dm-only]+[channel:]+[no-send]close instate/suppressed-team-results/, and the parent closes it silently with no record. Gaps stated there: the broker was a double, and it was a fresh process, not a launchd restart.Real post-restart witness, at this head (c3684ca): #5346 (comment). The actual launchd gateway service was booted out and bootstrapped onto this head against the live workspace; a real broker-attested Team task (
task-a092cc999635559953, from a reviewer's collaborator agent in Sutando Triage) got the guarded[dm-only]+[channel:]+[no-send]body; the outbox shows exactly oneno_sendclose, the Triage room and owner DM show zero posts of the witness text, andstate/suppressed-team-results/gained exactly one new matching record. The service was restored to normal afterward (SIGTERM at 04:37:32, bootstrap at 04:37:33, workspace-poller lock re-acquired at 04:37:37, core watcher ready after). The #5340 owner witness is a separate item.Docs
docs/claude-md-moved-detail.md(Result-marker semantics): the[thread:]entry now says the gateway's suppression journal reads the delivery verdict, so a guarded[dm-only]+[channel:]+skip close is recorded.Hardcoded paths
None added.
review-checkspassed.🤖 Generated with Claude Code