Repository navigation
fix(gateway): a skip after [channel:] on an owner result is a skip - #5340
Conversation
An owner task result led by `[channel: X]` then `[no-send]`, `[REPLIED]` or `[deduped:]` was posted into X as literal text: parse_markers keeps a skip after [channel:] as body (so the team guard can withhold a non-owner's redirect-plus-skip), the gateway re-stitched `[channel: X]` in front, and the broker's prefix-only skip check missed it. parse_markers gains skip_after_channel (default False). The gateway passes it on the guarded body at every delivery decision (task drain, orphan sweep, undelivered review, proactive route), so it alone decides the skip on the full leading block: nothing is posted, the lease closes no_send, the reason is logged. The guard still calls the plain parser, so a non-owner's redirect-plus-skip is withheld for review exactly as before. Voice (bodyIsSkipMarked) follows the same delivery verdict. Vendored copy synced; the shared case table gains delivery_skip. Closes #5331 Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@qingyun-wu review requested at head 587b9e4. This is the #5331 follow-up your agent found while reviewing #5326: an owner task's |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at 982b9657 (on Qingyun's ask to review and approve).
Why it's right: one flag on the one parser, default byte-identical. The guard keeps calling the plain parser, so a non-owner [channel:]+skip is still withheld for owner review (test c pins main's behaviour); the four delivery decisions (_post_ready_results, the orphan sweep, _quarantine_unsent, _proactive_route) pass skip_after_channel=True on the already-guarded body, so an owner's [channel: X]\n[no-send] closes the lease no_send instead of posting the marker text into room X; [deduped:] after [channel:] goes through the dedup plan. skip_marker_ownership.ts now mirrors that verdict for voice, and the shared fixture gains delivery_skip so the TS copy can't drift from parse_markers. This closes the nit I raised on #5326 (#5331).
Ran at this head (env -i, fresh worktree): gateway-channel-skip-owner (PASS, 0 failures), result-markers-thread, result-markers, team-result-guard, team-guard-suppress-journaled-silent-close, ag2-sparrow-drift, sparrow-result-guard, dm-result-skip-markers, bridge-marker-no-leak: all pass. CI: 36/36 green.
Still owed, not a reason to hold the approval: the before/after is the in-process wrapper harness (fake _req), not a live post-restart round trip. Since this changes the gateway's delivery decisions, a real [channel: X]\n[no-send] owner result through a restarted gateway, read back as "nothing posted, lease closed no_send", is the lesson-15 witness; same class as #5326's. Fine to post it on this PR after the fact.
Nit: docs/claude-md-moved-detail.md still describes the skip rule in one sentence; worth adding "after [channel:] the gateway treats it as a skip on the guarded body" so the owner-facing text matches.
john-the-dev
left a comment
There was a problem hiding this comment.
Post-merge review at 982b96577. It was merged at 02:40:57Z on one approval, and the PR body still lists the live witness as owed.
Verified:
- Marker grammar stays in
parse_markers, and the src and vendored copies are byte-identical.bridge-marker-no-leakpasses. - The new test fails 12 checks on
487d7af9and passes at the head. - Related suites are green: result-markers, team-result-guard, sparrow-result-guard, gateway thread-ask, outbox and the TS thread-ask test.
- Edge cases: Python and TS skip verdicts agree on 11 of them, including blank lines, both marker orders,
[thread:]between, mixed case, a marker mid-body, and an inline mention. - Mutants: M1–M4 and M6 are caught. Removing the flag in
_quarantine_unsent(:4611) is not.
Finding: "non-owner unchanged" does not hold with [dm-only].
- On a team task,
[dm-only]\n[channel: X]\n[no-send]\n…has no redirect action under the plain parse, so the guard delivers it unchanged. - The drain (
:4840, with the flag) then closes itno_send. But the suppression journal (:1180) keys on plainsuppression_only, so it writes nothing. - In the PR's own harness, a plain team
[no-send]writessuppressed-team-results/wr_*.json; this body writes none.[deduped:]behaves the same. - A guarded result now closes with no owner record. Filed as #5342, with a suggested fix and the missing case-table rows.
Please post the live witness.
|
Post-merge live witness on main Stand: Echo Act IV Pro |
Closes #5331
What changed, and why
An owner task result led by
[channel: X]then[no-send]/[REPLIED]/[deduped:]was posted into room X as the literal marker text:parse_markerskeeps a skip after[channel:]as body text, on purpose: the team guard reads that to withhold a non-owner's redirect-plus-skip for owner review (team-guard-suppress-journaled-silent-closecase e).[channel: X]back in front, so the broker's prefix-only skip check (ag2space-backend services/shared/queue/results.py_SKIP_MARKERS) missed it too.Fix, through the shared parser with no new parser copy:
parse_markers(text, skip_after_channel=False): the new flag makes a skip directly after[channel:]a skip. Default behaviour is byte-identical. The vendoredpackages/ag2-sparrowcopy is synced (ag2-sparrow-driftpasses).skip_after_channel=Trueon the guarded body at each delivery decision: the task drain (_post_ready_results), the orphan sweep, the undelivered-review path, and_proactive_route. Owner task: nothing is posted, the lease closesno_sendwith the canonical[no-send]/[REPLIED],[deduped:]goes through the dedup plan, and the reason is logged (archived <tid> (marker no-send, lease closed, not sent)).[channel:]+[no-send]is withheld for private owner review exactly as before. Test c below pins the outcome that main produces.bodyIsSkipMarkedinsrc/skip_marker_ownership.tsnow mirrors the delivery verdict. Before this change a voice task's[channel: !r:s]\n[no-send]\n…result was spoken (see the before output).tests/fixtures/thread-ask-cases.jsongainsdelivery_skip(defaultskip), checked by the Python parsers, the vendored copy and the TS helper.Look first at:
src/result_markers.py(the flag, about 3 lines), the fourparse_markers(..., skip_after_channel=True)sites inpackages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py, andsrc/skip_marker_ownership.ts.Not changed (possible follow-up):
[channel:]to the broker as a structured field instead of re-stitching it into the body. That would let the broker's own skip check see the real body. It is a protocol change and is not needed for correctness here, because the gateway now closes these withno_sendbefore anything reaches the broker.[channel:]+[no-send]posted through those bridges is still text. This PR is scoped to the gateway, as Owner-task result '[channel: X]' + '[no-send]' posts the literal [no-send] text to X #5331 is. The doc line says so. Filed as Other delivery bridges (Discord/Slack/Telegram/dm-result) still parse_markers() without skip_after_channel — same #5331 symptom #5341: those bridges' send helpers are shared across more callers than the gateway's were (at least one, Slack's, reused across an already-guarded path and a raw proactive path by its own docstring), so flipping the default there needs per-caller guard-timing verification first, not just a flag flip — real work across three bridge codebases, out of scope for this PR.tests/sparrow-result-guard.test.pysource-order check: the needle is nowparse_markers(bodyrather thanparse_markers(body). The intent is the same: the guard still runs before the parser.Before / after (production wrapper:
src/remote-gateway-bridge.pyloaded in-process, fake_req, same harness as #5326)Before, at parent
9c60c80aewith this PR's tests copied in:After, at head
587b9e453:Tests run (head
587b9e453, all underenv -i HOME=$HOME PATH=/usr/bin:/bin:/opt/homebrew/bin)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.sparrow-result-guardwas fixed above.remote-gateway-bridge.test.pyenv-fallback: no env token and no file yields empty tokenfails identically on parent9c60c80aeon this host, so it is unrelated.bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD): PASS.git diff --check: clean. eslint on the changed TS files: clean.Live witness
Owed. This changes the gateway delivery loop. The evidence above comes from the production wrapper in-process, not from a live post-restart round trip. A live witness should run once at the head the reviewers approve: an owner task answered with
[channel: <room>]\n[no-send]\n…, showing nothing posted in that room and thelease closed, not sentlog line.Docs
docs/claude-md-moved-detail.md(Result-marker semantics,[thread:]entry): the sentence about a skip after[channel:]now states the gateway and voice verdict and the guard's. The cataloglast_verifieddate is already 2026-10-10.Hardcoded paths
None added.
review-checkspassed.🤖 Generated with Claude Code