Repository navigation
fix(dm-result): honor the skip markers, not just the file markers - #2485
Conversation
`src/dm-result.py` is the REST-only fallback delivery path. `poll_dm_fallback`
in discord-bridge shells out to it for any `results/task-*.txt` that no channel
consumer claimed within 90s — so it is the LAST consumer in the result chain.
It already parses `[file:|send:|attach:]`, and its own comment gives the reason:
without parsing these markers it would deliver the literal text
`[file: /tmp/sutando-x.png]` in the DM
It did not parse the SKIP markers — `[no-send]`, `[REPLIED]`, `[deduped: …]` —
and that argument is stronger for them. A `[no-send]` body is not merely ugly in
a DM; its entire purpose is that it must not be delivered. Being last in the
chain, a marker this script ignores becomes exactly the DM the marker existed to
prevent, with the literal `[no-send]` text included.
CLAUDE.md documents `[no-send]` as "Discord bridge skips delivery for this task
(still archives)", and the bridge's own WS path honors it — ten modules import
`result_markers` (both bridges, task-bridge, slack, telegram, morning-briefing,
result_router, local_task_protocol, reply_chain, discord_addressee). dm-result
was the one consumer in that family that did not.
The check runs BEFORE the voice probe: a body marked do-not-deliver should not
be delivered by any path, and returning early also skips a needless SSE call.
Evidence — the new suite against this commit and its parent:
at this head:
Ran 6 tests OK
at the parent (fix reverted, tests unchanged):
Ran 6 tests FAILED (failures=3)
test_no_send_is_not_delivered AssertionError: a [no-send] body was
delivered to the owner's DM
test_replied_is_not_delivered AssertionError: a [REPLIED] body was
delivered again
test_deduped_is_not_delivered AssertionError: a [deduped:] body was
delivered
The three controls pass in BOTH states, so the suite is not vacuous: an ordinary
body is still delivered, a `[file:]` body is still delivered (attachment handling
unaffected), and prose that merely *mentions* `[no-send]` mid-sentence is still
delivered — the marker only counts at the start of the body, so this does not
reintroduce the failure mode #2481 fixes for `[dm-only]`.
Stand: Echo Act IV Mini
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 70%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed current head be25b854e84fe4c90241de5eb5a98a42258590cc.
No blocking findings in the code or local focused checks. dm-result.py now consumes the shared result-marker parser before the voice/network delivery path and exits on terminal skip actions, so [no-send], [REPLIED], and [deduped: ...] bodies cannot leak through the last-resort DM fallback when no earlier consumer claimed the result. Plain bodies, file-marker bodies, and prose that merely mentions a marker still reach the delivery path.
Checks: git diff --check origin/main...HEAD, py_compile with PYTHONPYCACHEPREFIX, python3 tests/dm-result-skip-markers.test.py, python3 tests/result-markers.test.py, python3 tests/bridge-marker-no-leak.test.py, python3 tests/bridge-audit-wiring.test.py, python3 -m ruff check src/dm-result.py tests/dm-result-skip-markers.test.py, and bash tests/review-checks.test.sh passed. GitHub checks are mostly green, with tsc + tests (clean install) still pending, so I am leaving this as a review comment rather than an approval until that current-head CI finishes.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed current head be25b854e84fe4c90241de5eb5a98a42258590cc.
No blocking findings. The current-head CI that was still pending during my first pass has completed green, and the local focused checks already covered the skip-marker boundary. dm-result.py now honors terminal result markers before the voice/network delivery path, so [no-send], [REPLIED], and [deduped: ...] bodies cannot leak through the last-resort DM fallback; ordinary text, file markers, and prose merely mentioning a marker remain deliverable.
Checks: git diff --check origin/main...HEAD, py_compile with PYTHONPYCACHEPREFIX, python3 tests/dm-result-skip-markers.test.py, python3 tests/result-markers.test.py, python3 tests/bridge-marker-no-leak.test.py, python3 tests/bridge-audit-wiring.test.py, python3 -m ruff check src/dm-result.py tests/dm-result-skip-markers.test.py, bash tests/review-checks.test.sh, and current-head GitHub CI passed.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head be25b854e84fe4c90241de5eb5a98a42258590cc after the previously pending CI completed. The full dm-result.py fallback path now parses shared result markers before voice/network delivery and returns on terminal skip actions, so [no-send], [REPLIED], and [deduped: ...] cannot leak through the last-resort DM consumer. Ordinary bodies, attachment markers, and inline prose mentions still deliver.
Reviewer-run tests/dm-result-skip-markers.test.py (6/6), tests/result-markers.test.py (34/34), bridge marker/audit suites, Ruff, diff check, and REVIEW.md path scan pass. All exact-head GitHub checks and CLA are green. The worst-case regression—silently suppressing ordinary output—is bounded by the positive delivery controls.
Verdict: approved; code-ready on this head.
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — verified independently.
What I checked
- Confirmed the API the fix leans on:
result_markers.parse_markers(text).actionsyieldsAction(kind="skip", value∈{no-send,REPLIED,deduped}), and skip markers are start-anchored (^\s*\[no-send\]etc.) and terminal (src/result_markers.py:103-107, 138-186). Sonext(a for a in ….actions if a.kind=="skip")correctly fires only when the body starts with a skip marker — prose that merely mentions[no-send]is untouched. - Placement is right: the check sits after
textis assembled and beforevoice_connected()and any send path, so the last-consumer-in-the-chain leak the PR describes can't reach the DM. Early-returnmatches the existing voice/file-marker early-return style in the same function. - Ran the new test on the PR head in an isolated worktree: 6/6 pass — both directions covered (no-send/REPLIED/deduped suppressed; plain body, file-marker body, and prose-mentioning-marker body still delivered).
- CI: all green (diff-cover ≥95%, tsc+tests, smoke, CLA met).
Correct, minimal, well-tested, closes a real do-not-deliver leak. LGTM.
|
@cla-assistant check |
What's wrong
src/dm-result.pyis the REST-only fallback delivery path.poll_dm_fallback(discord-bridge) shells out to it for anyresults/task-*.txtthat no channel consumer claimed within 90s — making it the last consumer in the result chain.It already parses
[file:|send:|attach:], and its own comment states why:It did not parse the skip markers —
[no-send],[REPLIED],[deduped: …]— and that argument is stronger for them. A[no-send]body is not merely ugly in a DM; its entire purpose is that it must not be delivered. Because this script is last in the chain, a marker it ignores becomes exactly the DM the marker existed to prevent, with the literal[no-send]text included.CLAUDE.mddocuments[no-send]as "Discord bridge skips delivery for this task (still archives)", and the bridge's WS path honors it. Ten modules importresult_markers— both bridges,task-bridge, slack, telegram, morning-briefing,result_router,local_task_protocol,reply_chain,discord_addressee.dm-resultwas the one consumer in that family that didn't.How I hit it
Not hypothetically. I was about to write a probe task for an unrelated evidence capture and checked whether its result would reach the owner. It would have —
task-*is inFALLBACK_PREFIXES, and[no-send]would not have saved it.The change
Parse markers and return early on a skip action, before the voice probe — a body marked do-not-deliver shouldn't be delivered by any path, and returning early also skips a needless SSE call.
Evidence
The new suite at this head and at the parent, tests unchanged:
The three controls pass in both states, so the suite isn't vacuous:
[file:]body is still delivered (attachment handling unaffected)[no-send]mid-sentence is still delivered — the marker only counts at the start of the body, so this does not reintroduce the failure mode fix(markers): stop [dm-only] editing prose that merely mentions it #2481 fixes for[dm-only]Each case asserts the marker path is taken without reaching the network — the test never supplies a token, so a real send attempt would be visible.
🤖 Generated with Claude Code