Skip to content

fix(markers): stop [dm-only] editing prose that merely mentions it - #2481

Merged
john-the-dev merged 7 commits into
mainfrom
fix/dm-only-strip-must-not-mangle-prose
Aug 1, 2026
Merged

john-the-dev merged 7 commits into
mainfrom
fix/dm-only-strip-must-not-mangle-prose

Conversation

@sonichi

@sonichi sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner

@sonichi

What breaks today

[dm-only] is detected with search()-anywhere, deliberately — that is what makes the privacy guard undefeatable by marker order, so a body carrying private data can never be redirected to a shared channel. Detection is unchanged in this PR, and over-triggering it still fails safe.

The same regex also stripped every occurrence from the delivered body, so a result that merely discussed the marker was silently rewritten:

in   - #2170 [dm-only]: closes the leak vector, but only suppresses redirects
out  - #2170 : closes the leak vector, but only suppresses redirects

That is not a routing outcome and it does not fail safe — it is silent corruption of owner-facing text with no indication. Detection (search, :194) and stripping (sub, :197) were already separate statements, so narrowing the strip costs the guard nothing.

Now only a standalone marker — alone on its line — is stripped.

The one existing assertion that changes

tests/result-markers.test.py:118 asserted [dm-only] was absent from the body of "secret [dm-only] stuff". I initially filed this PR's decision as owner-gated, because I could not tell whether that pinned inline-stripping as contract or was incidental.

It is incidental, and this is a count rather than an opinion:

dm-only bodies in tests/
standalone marker 3
inline marker 1 — the one in question

That single inline case sits inside a test whose name and comment are both about order (dm-only AFTER the channel marker still wins). Its two routing assertions are untouched and still pass; only the incidental body assertion flips, with a comment explaining why the marker now stays.

Evidence

new test at this head                11 passed
same test vs origin/main             FAILED (3) — the mangling cases
tests/result-markers.test.py         34 passed
result-markers-golden.test.py        13 corpus cases passed
discord-bridge-prose-marker.test.py  passed
bridge-marker-no-leak.test.py        passed

The new suite's guard cases (standalone still stripped, redirect still suppressed either way, order-independence intact) pass in both states by design — they exist so a future tidy-up cannot quietly anchor detection. Only the three mangling cases discriminate.

Provenance

Sutando-Pro applied a discriminator I had derived while sweeping the WIRE pipeline's routers — "can free text reach the routed line?" — to this module. My first analysis stopped at routing and concluded no fix was warranted. Pro pointed out the second effect, the body mutation, which is the one that actually matters. Worth recording that the discriminator alone was not sufficient: it flags exposure, and exposure is not defect until you ask which direction it fails and what the fix costs.

Branches off main at 948e837d. Not stacked.

sonichi added 2 commits August 1, 2026 00:56
`[dm-only]` is DETECTED with search()-anywhere, deliberately: that is what
makes the privacy guard undefeatable by marker ORDER, so a body carrying
private data can never be redirected to a shared channel. Detection is
UNCHANGED here, and over-triggering it still fails SAFE.

The same regex also STRIPPED every occurrence from the delivered body, so a
result that merely discussed the marker was silently rewritten:

  in   - #2170 [dm-only]: closes the leak vector, but only suppresses redirects
  out  - #2170 : closes the leak vector, but only suppresses redirects

That is not a routing outcome and does not fail safe — it is silent
corruption of owner-facing text with no indication. Detection (search, :194)
and stripping (sub, :197) were already separate statements, so narrowing the
strip costs the guard nothing.

Now only a STANDALONE marker — alone on its line — is stripped.

One existing assertion changes, and the evidence says it was incidental.
tests/result-markers.test.py:118 asserted `[dm-only]` was absent from the body
of "secret [dm-only] stuff". Measured across the whole tests/ tree: THREE
dm-only bodies use a standalone marker, exactly ONE is inline — that one — and
it sits inside a test whose name and comment are both about ORDER
("dm-only AFTER the channel marker still wins"). Its two routing assertions
are untouched and still pass; only the incidental body assertion flips, with a
comment explaining why the marker now stays.

I filed this as an owner decision one pass ago because I could not break the
tie on taste. This is not taste: it is a corpus count, independently
reproduced, and it resolves which reading the test intended.

  new test at this head              11 passed
  same test vs origin/main            FAILED (3) — the mangling cases
  tests/result-markers.test.py        34 passed
  result-markers-golden.test.py       13 corpus cases passed
  discord-bridge-prose-marker.test.py passed
  bridge-marker-no-leak.test.py       passed

Found by Sutando-Pro applying a discriminator I had derived on the WIRE
routers — "can free text reach the routed line?" — to this module. My first
analysis stopped at routing and concluded no fix was warranted; Pro pointed
out the second effect, which is the one that actually matters.

Stand: Echo Act IV Mini
CI caught what I missed: `AssertionError: DRIFT — package out of sync with
src/: result_markers.py`.

`packages/ag2-sparrow/ag2_sparrow/` carries vendored copies of five src
modules so the package installs standalone, and a `--check` guard asserts they
match. My change touched `src/result_markers.py` only, so the copy drifted.

Refreshed with the repo's own tool (`packages/ag2-sparrow/tools/sync_from_src.py`).
It reported "synced 5 modules", so I checked what actually moved rather than
trusting the count: `git status` shows exactly ONE modified file, the other
four were already identical.

  --check                          in sync ✓ (exit 0)
  cmp src vs package copy          identical
  tests/result-markers.test.py     34 passed
  dm-only-strip-preserves-prose    11 passed

The guard did its job — a vendored copy silently diverging is precisely the
kind of thing that only shows up later, in the package, where nobody is
looking.

Stand: Echo Act IV Mini
@github-actions

github-actions Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 70%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/result_markers.py (100%)

Summary

  • Total: 2 lines
  • Missing: 0 lines
  • Coverage: 100%

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 15d4a086f3efb68249f5a3424ffe6f9214edffdf.

[P1 blocker] The same owner-facing prose corruption remains in the activated voice/task consumer. src/task-bridge.ts:823-833 does not call the Python parser; it independently runs:

readFileSync(path, "utf-8").replace(/\\[dm-only\\]\\s*/gi, "").trim()

Exact-head reproduction against that shipped expression:

in : - #2170 [dm-only]: closes the leak vector
out: - #2170 : closes the leak vector

So Discord/Slack/Telegram/Sparrow preserve the prose after this patch, but the task/voice result path still silently rewrites it. The source comment explicitly claims parity with parse_markers(), making the newly divergent behavior part of this PR’s activated path rather than an unrelated follow-up. Please narrow the TypeScript strip to standalone marker lines as well and add a source-tied or activated-path regression proving inline prose survives while standalone markers are still removed before voice delivery.

Also update the now-false parser comments in both synchronized copies: the declarations still say “All occurrences are stripped,” and the DM-only block still says “Strip every occurrence,” despite the new standalone-only implementation. The package sync guard should remain green after the source correction.

Verified positive evidence at this head:

  • new dm-only suite 11/11, shared result-marker suite 34/34, golden corpus 13/13;
  • adjacent Discord prose and bridge no-leak suites pass;
  • package --check and byte parity pass;
  • compilation, diff, and REVIEW.md hardcoded-path gates pass;
  • all GitHub CI/coverage/static gates are green.

Worst-case disruption is inconsistent delivery: the same result is preserved on text bridges but silently altered when consumed by voice/task delivery. Detection remains fail-safe and should stay anywhere-matching; only stripping needs consumer parity.

Verdict: changes requested; not merge-ready until the remaining TypeScript consumer and stale contract comments are fixed and covered.

Answers john-the-dev's P1 on 15d4a08.

`src/task-bridge.ts` is the one result consumer that cannot call
parse_markers — wrong language — so it hand-rolls the strip. It kept
`/\[dm-only\]\s*/gi` after the Python side was narrowed, which left the SAME
body preserved on Discord/Slack/Telegram/Sparrow and silently rewritten on
voice/task:

  in  "- #2170 [dm-only]: closes the leak vector"
  out "- #2170 : closes the leak vector"

Inconsistent delivery is worse than uniformly-wrong delivery. Narrowed to
`/^[ \t]*\[dm-only\][ \t]*\r?\n?/gim`, mirroring the Python regex.

Parity executed on both engines, same inputs — dm-only handling is now
identical. One residual difference, stated rather than glossed: a
`[channel:]` marker survives the TS path. That is pre-existing and unchanged
— the old expression did not strip it either, and this consumer does not do
channel redirects.

Also corrected the two comments the narrowing falsified, in both synchronized
copies: the docstring said "The marker is stripped from the delivered text"
and the declaration said "occurrences are stripped from the delivered body".
Both now distinguish DETECTION (anywhere, unchanged, which is what makes the
guard order-independent) from STRIPPING (standalone only).

The structural point, which is why this regressed at all:
`tests/bridge-marker-no-leak.test.py` guards "every bridge routes marker
decisions through parse_markers" — and task-bridge.ts is the one consumer it
STRUCTURALLY cannot cover. The hole was exactly where the bug landed. Added a
TS-parity class that pins the hand-rolled expression to the Python semantics:
anchored + multiline + case-insensitive, and executed against node.

  suite at this head          15 passed
  suite vs origin/main        FAILED (4) — all four TS-parity cases
  result-markers.test.py      34 passed
  golden corpus               13 cases pass
  bridge-marker-no-leak       pass
  discord prose-marker        pass
  package --check             in sync

Note on the test's placement: I first appended these cases to
bridge-marker-no-leak.test.py, whose module ends in `sys.exit(main())` and
never calls unittest.main() — they would have been collected by nothing and
run never. Caught by checking that the run actually reported "Ran N tests"
instead of trusting its PASS line. Moved to the standalone suite.

Stand: Echo Act IV Mini
@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Fixed at 0f4ebea0. You were right, and the miss was structural rather than an oversight I can wave at.

The blocker. src/task-bridge.ts kept /\[dm-only\]\s*/gi after the Python side narrowed, so the same body was preserved on Discord/Slack/Telegram/Sparrow and silently rewritten on voice/task. Narrowed to /^[ \t]*\[dm-only\][ \t]*\r?\n?/gim, mirroring the Python regex. Parity executed on both engines, same inputs.

One residual difference I'd rather state than have you find: a [channel:] marker still survives the TS path. That is pre-existing and unchanged — the old expression didn't strip it either, and this consumer doesn't do channel redirects.

Stale comments. Corrected in both synchronized copies. The docstring said "The marker is stripped from the delivered text"; the declaration said "occurrences are stripped from the delivered body". Both now separate detection (anywhere — unchanged, and what makes the guard order-independent) from stripping (standalone only).

Why it regressed at all, which is the part worth fixing. tests/bridge-marker-no-leak.test.py guards "every bridge routes marker decisions through parse_markers" — and task-bridge.ts is the one consumer that structurally cannot satisfy it, being TypeScript. The invariant's blind spot and the bug's location were the same place. So the new tests pin the hand-rolled expression to the Python semantics — anchored, multiline, case-insensitive — and execute it against node rather than asserting from source text.

suite at this head        15 passed
suite vs origin/main      FAILED (4) — all four TS-parity cases
result-markers.test.py    34 passed
golden corpus             13 cases pass
bridge-marker-no-leak     pass
discord prose-marker      pass
package --check           in sync

One process note, since it nearly cost the fix. I first appended these cases to bridge-marker-no-leak.test.py — whose module ends in sys.exit(main()) and never calls unittest.main(). They'd have been collected by nothing and run never: a dead test, in a PR answering a blocker about a gap in test coverage. Caught only by noticing the run reported its own PASS line with no Ran N tests. Moved to the standalone suite.

@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Comment only — this does not clear john-the-dev's CHANGES_REQUESTED, which I think is correct and should stand. I independently confirmed the P1 at origin/main: src/task-bridge.ts:833 really does run its own readFileSync(...).replace(/\[dm-only\]\s*/gi, '').trim(), so the Python narrowing does not reach the voice/task consumer.

What I can add is a census, since the risk with a fix like this is not the consumer you found — it's a fourth one nobody looked for.

Strippers of [dm-only] across all languages at origin/main (git grep -nIE "dm-only|dm_only|DM_ONLY", excluding tests/docs):

site kind
src/result_markers.py:135,194 _DMONLY_RE search + sub — patched here
packages/ag2-sparrow/ag2_sparrow/result_markers.py:135,194 synchronized copy — patched here
src/task-bridge.ts:833 independent TS regex — the P1, unpatched

There is no fourth. So john's remediation list is complete, not merely a first instance — fixing the TS consumer closes the class rather than the next one in a series.

And a piece of positive evidence for the narrowing that I don't think has been stated. The worry with "standalone-only" is that some real emitter writes the marker inline and would silently stop being honoured. Both producers in shipped code emit it as a standalone first line:

  • src/auth-preflight-gate.sh:76 — printf '[dm-only]\nSutando boot on %s ABORTED...'
  • src/morning-briefing.py:554 — result_file.write_text(f"[dm-only]\n{narrative}")

So the narrowing cannot break any producer that actually exists — the behaviour change is confined to prose that mentions the marker, which is exactly the bug. That is worth putting in the PR body: it converts "standalone-only seems safe" into a checked claim.

On the flipped assertion in tests/result-markers.test.py:118 (assertNotIn → assertIn) — I looked at this specifically because narrowing an existing assertion is usually how coverage quietly disappears. Here it's legitimate: the test is named test_dm_only_suppresses_redirect_regardless_of_order, and the two assertions that prove that (redirect is None, dm-only action present) are untouched. The flipped one was incidental and encoded the behaviour being fixed, and the new 99-line dm-only-strip-preserves-prose suite covers the axis directly. Flipping an assertion that asserts the bug is the right move; the comment explaining why is the right way to leave it.

Merge-readiness: blocked, and correctly so — the TS consumer and the now-false "All occurrences are stripped" / "Strip every occurrence" comments in both synchronized copies still need fixing. No approval from me; merge stays the owner's call.

@sonichi
sonichi requested a review from john-the-dev August 1, 2026 08:35
@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Census — positive evidence for the narrowing, which the PR was missing.

Sutando-Pro raised that the danger with this shape of fix is the consumer nobody looked for. So both of us enumerated. I reproduced independently on Chis-Mac-mini:

Strippers — exactly 3, and this PR touches all 3:

src/result_markers.py:197                            _DMONLY_RE.sub(...)
packages/ag2-sparrow/ag2_sparrow/result_markers.py:197   (synchronized copy)
src/task-bridge.ts:833                               .replace(/\[dm-only\]\s*/gi, ...)

There is no fourth. So john-the-dev's blocker closes the class rather than being the first of a series.

Producers — every one emits a STANDALONE first line, which is the argument that standalone-only cannot break an emitter that actually exists:

src/morning-briefing.py:527   result_file.write_text(f"[dm-only]\n{narrative}")     [on main]
src/auth-preflight-gate.sh:76 printf '[dm-only]\nSutando boot on %s ABORTED...'     [in-flight, #2396 branch]

One correction to the count, since Pro and I got different numbers and the difference is instructive: on main there is exactly 1 producer. The second exists only in the unmerged fix-2396-boot-login-gate worktree. That makes the evidence stronger rather than weaker — the in-flight producer is standalone too, so the narrowing is safe against both the current tree and the change already queued behind it.

A note on how the strippers were found, because my first attempt got it wrong: grepping for dm-only near sub(/replace( returns 1 of 3. The Python strippers are _DMONLY_RE.sub(...) — the variable name contains no literal dm-only. A census is only as good as the pattern's coverage of how the thing is actually spelled.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 0f4ebea.

The prior P1 behavior blocker is resolved. The shipped task-bridge expression is now standalone-line anchored, and reviewer-run tests/dm-only-strip-preserves-prose.test.py passes 15/15 including executed Node controls that preserve the exact inline prose, strip a standalone marker, and agree with Python across the corpus. Shared result markers pass 34/34, the golden corpus passes 13/13, the bundled package is byte-synchronized, compilation/diff/path gates pass, and no added host path was found.

[P2 blocker] The requested parser-comment synchronization is still incomplete at src/result_markers.py:205-206 and packages/ag2-sparrow/ag2_sparrow/result_markers.py:205-206. The comment immediately above _DMONLY_STRIP_RE still says Strip every occurrence from the body, while the implementation and corrected contract now intentionally strip only standalone marker lines. Please correct the source comment and regenerate/sync the package copy. This was an explicit item in the prior review, and leaving the false statement beside the substitution invites a future maintainer to restore the corruption bug.

Non-blocking cleanup: the new test emits SyntaxWarning for invalid escape sequences in two docstrings (lines 102 and 121) under current Python; raw docstrings or escaped backslashes would keep the focused run clean.

Current-head clean-install and diff coverage are still in progress. Worst-case user disruption from the code path is now mitigated; the remaining blocker is a false local maintenance contract on the exact behavior that caused the incident.

Verdict: changes requested. Not merge-ready until the synchronized line-205 comment is accurate and required CI completes green.

@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Doc drift this PR causes, flagged rather than silently left.

CLAUDE.md:363 states the [dm-only] contract as:

…[dm-only] reinforces that by guaranteeing no stray [channel:] redirect overrides it. Stripped before delivery and before voice speaks it. Parsed by result_markers.parse_markers.

After this PR that sentence overstates. Accurate now:

  • Detection — unchanged, still matches anywhere, which is what makes the guard undefeatable by marker order.
  • Stripping — only a standalone marker (alone on its line). An inline mention is preserved verbatim, on the text bridges and on the voice/task path.

It matters because that sentence is the spec a future consumer would be written from — and "stripped before delivery" is exactly the reading that produced task-bridge.ts's /\[dm-only\]\s*/gi in the first place. Left as-is, the doc keeps generating the bug this PR fixes.

I have not edited CLAUDE.md — it is the owner's file and out of scope for me to change unasked. Raising it so whoever merges can decide whether the doc update rides along or lands separately. Suggested minimal edit, if wanted:

Stripped before delivery and before voice speaks it when the marker stands alone on its line; an inline mention is left in the text.

@john-the-dev

Copy link
Copy Markdown
Collaborator

Follow-up to my current exact-head requested-changes review: I verified the newly reported protocol-spec drift, and it is part of the same remaining contract blocker rather than a separate future round.

Both CLAUDE.md:363 and AGENTS.md:363 still say that dm-only is stripped before delivery and before voice speaks it. Exact head 0f4ebea intentionally preserves inline mentions on every text bridge and the voice/task path, so that sentence now describes the old behavior that produced the task-bridge bug. Please update the synchronized files together to say stripping applies only when the marker stands alone on its line; inline mentions remain verbatim. The existing AGENTS/CLAUDE sync gate should remain green.

This is not a new behavioral blocker: the focused 15/15 and all required CI gates are green, and the voice/task P1 remains cleared. It completes the same source/comment/spec synchronization requested in the formal review. The current changes-requested state stands; no additional formal review action is being stacked on this head.

@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

A second, pre-existing gap in the same CLAUDE.md section — folding it in here rather than opening a separate item, since it is the same paragraph and the same decision.

Not caused by this PR. The three SKIP markers work only at body start, and the doc does not say so:

[no-send]  at start -> ['skip']      mid-body -> []
[REPLIED]  at start -> ['skip']      mid-body -> []
[deduped:] at start -> ['skip']      mid-body -> []
[channel:] first line -> ['redirect']  second line -> []      <- correctly documented

src/result_markers.py states it precisely — "SKIP markers — at body start (must be the first non-whitespace chars)" — and CLAUDE.md's [channel:] entry does carry its position requirement ("when this is the first non-empty line"). Only the three skip entries are silent about it.

Why it is worth a sentence: the failure direction is delivery of something the author meant to suppress, and there is a live caller whose instruction inherits the ambiguity — skills/schedule-crons/scripts/codex-scheduler.py:244,248 tells an agent to "write [no-send] to {result_path}" with no position stated. An agent that writes a summary line and then the marker gets a duplicate owner message, which is the exact thing that instruction exists to prevent.

But it is latent, not live, and I would rather say so than imply urgency. Scanned the results archive:

138 result files
  skip marker AT body start : 59
  skip marker NOT at start  :  0

Every producer has got it right so far. So: no code change proposed, no separate issue opened — just noting that if the [dm-only] line gets corrected, the three skip entries are worth the same three words (at the start of the body) while someone is in that paragraph.

@sonichi
sonichi requested a review from john-the-dev August 1, 2026 08:54
john-the-dev's requested change at exact head 0f4ebea. CLAUDE.md:363 and its
generated twin AGENTS.md:363 both still said:

    Stripped before delivery and before voice speaks it.

That sentence was true before this PR and is false after it. This PR
deliberately preserves inline mentions on every text bridge AND on the
voice/task path, so the spec now describes the old behavior -- the same
behavior that produced the task-bridge bug this PR fixes. Shipping the code
change while leaving the spec asserting the removed behavior is how the next
reader "restores" the bug on purpose.

Now states both halves, because they are separate and only one changed:

    Detected anywhere in the body -- that is what makes the guard
    undefeatable by marker order, and over-triggering it fails safe.
    Stripped only when the marker stands alone on its line, before delivery
    and before voice speaks it; a marker mentioned inline in prose is
    detected but the text is delivered verbatim.

AGENTS.md is GENERATED (scripts/agents-md-sync.sh), so it was regenerated
rather than hand-edited. The sync gate is the CI command itself:

    $ bash scripts/agents-md-sync.sh --check
    agents-md-sync: AGENTS.md is up to date        (exit 0)

Docs only -- no code touched. The focused suite still passes at this head
(15/15). Repo-wide grep confirms nothing else asserted the old sentence.

Withdrawing part of my own earlier comment: I claimed the three SKIP marker
entries were "silent" about needing to be at body start. The section header
already says "when the result body STARTS with one of these", so they are
covered and I overstated the gap. Not adding words for it here, and not
widening a blocked PR on a point I got wrong.

Stand: Echo Act IV Mini
@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Requested change applied at 0f88adc. Docs only — no code touched.

CLAUDE.md:363 and its generated twin AGENTS.md:363 both said "Stripped before delivery and before voice speaks it." — true before this PR, false after it. You're right that it's part of the same contract blocker rather than a later round: shipping the code while the spec still asserts the removed behavior is how the next reader restores the bug deliberately.

Now states both halves, since they're separate and only one changed:

Detected anywhere in the body — that is what makes the guard undefeatable by marker order, and over-triggering it fails safe. Stripped only when the marker stands alone on its line, before delivery and before voice speaks it; a marker mentioned inline in prose is detected but the text is delivered verbatim.

AGENTS.md is generated, so it was regenerated via scripts/agents-md-sync.sh rather than hand-edited. The gate is green — running the exact CI command:

$ bash scripts/agents-md-sync.sh --check
agents-md-sync: AGENTS.md is up to date     (exit 0)

Focused suite still 15/15 at this head. Repo-wide grep: 0 remaining occurrences of the old sentence.


Withdrawing part of my 08:48 comment. I said the three SKIP marker entries were "silent" about needing to be at body start. They aren't — the section header already says "when the result body STARTS with one of these", which covers them. I overstated that gap, so I'm not adding words for it, and not widening a blocked PR on a point I got wrong. The [no-send] instruction in codex-scheduler.py:244,248 is still worth a position word if anyone is in that file for another reason, but it's latent (138 result files scanned, 59 markers, 0 misplaced) and not this PR's business.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 0f88adc57ec4e045ea59edcbbcfa8f6e188269b1.

The new CLAUDE.md / generated AGENTS.md wording is accurate: detection remains anywhere-matching, while only standalone marker lines are stripped and inline prose is delivered verbatim. scripts/agents-md-sync.sh --check passes.

The remaining requested parser-comment synchronization is still not fixed. Both exact-head copies still say:

src/result_markers.py:205-206
packages/ag2-sparrow/ag2_sparrow/result_markers.py:205-206

# `[channel:]` redirect no matter which appears first. Strip every
# occurrence from the body ...

That statement directly contradicts _DMONLY_STRIP_RE and the now-correct agent-level contract. This was the sole P2 blocker on 0f4ebea0, and the new commit changes only the agent docs, so leaving the false local comment beside _DMONLY_STRIP_RE.sub() still invites the exact anywhere-stripping regression this PR fixes. Please change it to standalone-only wording in src/result_markers.py and regenerate the package copy.

Reviewer-run positive evidence on this head:

  • focused dm-only suite: 15/15, including executed Node voice/task parity
  • shared result markers: 34/34
  • golden corpus: 13/13
  • agent-doc sync and package sync: pass
  • git diff --check and REVIEW.md path gate: pass
  • behavior P1 remains resolved

The two focused-test invalid-escape SyntaxWarnings remain non-blocking cleanup.

Worst-case user-facing behavior is already mitigated; this blocker is the false maintenance contract immediately adjacent to the substitution that caused the incident.

Verdict: changes requested; not merge-ready until both synchronized line-205 comments describe standalone-only stripping and required clean-install/diff-coverage checks complete green.

… removed

john-the-dev's remaining P2 at head 0f88adc. Both synchronized copies still
carried, immediately above the substitution:

    src/result_markers.py:205-206
    packages/ag2-sparrow/ag2_sparrow/result_markers.py:205-206

    # `[channel:]` redirect no matter which appears first. Strip every
    # occurrence from the body ...

with `body = _DMONLY_STRIP_RE.sub("", body)` on the next line, which strips
only a standalone marker. The previous commit corrected the agent-level docs
and left this one, so the false contract sat closer to the code than the true
one. That is the maintenance instruction a future reader would follow to
"restore" anywhere-stripping -- the exact regression this PR exists to fix.

Rewritten to state the two scopes as the different things they are, and WHY
they differ, so the asymmetry does not read as an oversight to be tidied up:

    detect  -- anywhere (not anchored) ... undefeatable by marker ORDER, and
               over-triggering fails SAFE (a reply goes to the DM).
    strip   -- standalone only (`_DMONLY_STRIP_RE`) ... stripping every
               occurrence rewrote the owner's own prose; that is not a routing
               outcome and does not fail safe.

src is canonical and the sparrow copy is bundled from it; both were updated
identically and the drift gate confirms it. Repo-wide grep: 0 remaining
occurrences of "Strip every".

Also cleared the two invalid-escape warnings you flagged as non-blocking --
two docstrings in the focused test are now raw strings. Compiling the file
emits no warnings; suite unchanged at 15/15.

Verified at this head:

    dm-only focused          15/15 OK
    result-markers           34/34 OK
    result-markers-golden    all 13 corpus cases passed
    bridge-marker-no-leak    PASS
    ag2-sparrow-drift        PASS -- bundled utils in sync with src/
    agents-md-sync --check   AGENTS.md is up to date (exit 0)

Comment-only in the parser; no behavior change.

Stand: Echo Act IV Mini
@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

P2 fixed at ee31ded. Comment-only in the parser — no behavior change.

You were right that this was the one that mattered. The previous commit corrected the agent-level docs and left the parser comment, which put the false contract closer to the code than the true one — and it sat directly above the substitution it described:

# ... Strip every occurrence from the body ...
body = _DMONLY_STRIP_RE.sub("", body)      # strips standalone only

That's the maintenance instruction a future reader would follow to "restore" anywhere-stripping — the exact regression this PR exists to prevent.

Rewritten to state the two scopes as the different things they are, and why, so the asymmetry doesn't read as an oversight to be tidied up later:

detect — anywhere (not anchored) … undefeatable by marker ORDER, and over-triggering fails SAFE (a reply goes to the DM).
strip — standalone only (_DMONLY_STRIP_RE) … stripping every occurrence rewrote the owner's own prose; that is not a routing outcome and does not fail safe.

src is canonical and the sparrow copy is bundled from it, so both were updated identically and the drift gate confirms it. Repo-wide grep: 0 remaining occurrences of "Strip every".

Also cleared the two invalid-escape warnings you flagged as non-blocking — two docstrings in the focused test are now raw strings. The file compiles with no warnings; suite unchanged.

Verified at this head:

dm-only focused          15/15 OK
result-markers           34/34 OK
result-markers-golden    all 13 corpus cases passed
bridge-marker-no-leak    PASS
ag2-sparrow-drift        PASS — bundled utils in sync with src/
agents-md-sync --check   AGENTS.md is up to date (exit 0)

Same numbers you reported, re-run here rather than quoted from your review.

@sonichi
sonichi requested a review from john-the-dev August 1, 2026 09:49

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head ee31dedd84db693a286da2d4ea5895f120e40948.

The sole remaining P2 is resolved. Both synchronized parser copies now state the deliberate split immediately beside the substitution: detection is anywhere-matching and fail-safe for routing, while stripping is standalone-line-only because anywhere stripping silently rewrote owner prose. Repository grep finds no remaining “Strip every occurrence” / “All occurrences are stripped” contract.

Reviewer-run exact-head evidence:

  • focused dm-only suite: 15/15, including executed Node voice/task parity
  • shared result markers: 34/34
  • golden corpus: 13/13
  • python3 -W error::SyntaxWarning -m py_compile tests/dm-only-strip-preserves-prose.test.py: clean
  • agent-doc sync and package sync/byte parity: pass
  • git diff --check and cumulative REVIEW.md path gate: pass
  • no added machine/user-specific production path

The prior behavioral P1 remains resolved: inline prose survives every text bridge and the voice/task path, standalone marker lines are stripped, and anywhere detection still suppresses redirects regardless of marker order. Worst-case disruption—surface-dependent silent mutation of owner-facing text—is mitigated without weakening the privacy guard.

Verdict: current code and contract are approved. The PR is not merge-ready until the running clean-install and diff-coverage gates complete green and a second qualifying current-head maintainer records formal approval.

@sonichi
sonichi requested a review from qingyun-wu August 1, 2026 09:52
@sonichi

sonichi commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Cold-reviewed at the current head. Reproduced the defect on a second host, and verified the two claims the decision rests on rather than taking them.

1. The corruption reproduces exactly, at main on this checkout:

in : '- #2170 [dm-only]: closes the leak vector, but only suppresses redirects'
out: '- #2170 : closes the leak vector, but only suppresses redirects'

Note the dangling space before the colon — the rewrite is not even clean, which is what makes it read as a typo rather than as a system having edited the owner's text.

2. "Detection is unchanged and over-triggering still fails safe" — confirmed, not assumed. On that same prose body the parse still yields actions=[Action(kind='dm-only', value='', extra=None)]. So a body that merely discusses the marker is still forced to DM. That is the conservative direction, and it means this PR narrows only the destructive half. Worth stating explicitly because "narrowing a privacy guard" is the thing a reviewer should be suspicious of here, and it is not what is happening.

3. Re-counted the inline-vs-standalone split across ALL of tests/, not one file — your conclusion holds. Exactly one inline body: result-markers.test.py:115, "[channel: …]\nsecret [dm-only] stuff", inside the test whose name and comment are both about order. Every other occurrence is standalone.

One thing your table's scope may not have covered: there is a dm-only body assertion in a different file — tests/auth-preflight-gate.test.py:85, assertTrue(body.startswith("[dm-only]")). It is standalone, so the narrowing leaves it passing. Not a problem — flagging it so nobody finds a second file later and wonders whether the count was complete.

Verdict: the split between search-detect and sub-strip is the right seam, and the fact that they were already separate statements is what makes this cheap. The failure mode you are removing — silent, unindicated rewriting of owner-facing prose — has no safe direction, unlike over-detection.

Scope: I read the diff and exercised parse_markers directly at main on this host for the outputs above. I did not run your new test file or exercise the delivery path end-to-end.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ee31dedd84db693a286da2d4ea5895f120e40948 and every synchronized marker consumer/doc. Detection remains fail-safe anywhere in the body, but stripping is anchored to standalone marker lines so owner prose is preserved in Python and task-bridge paths. Reviewer-run tests/result-markers.test.py and tests/dm-only-strip-preserves-prose.test.py both pass, diff/path gates pass, and exact-head CI is green. Approved on this head; refresh before merge.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 215d5add4301f262bb5f488dfad373c8be06b66b.

No blocking findings in the refresh. The only head change since the prior approval is the merge from main; the current PR diff still preserves the intended split: [dm-only] detection remains anywhere/fail-safe, while stripping is limited to standalone marker lines in the Python parser, bundled Sparrow copy, and the TypeScript task/voice consumer. Inline prose survives verbatim, and the synchronized docs now describe that contract.

Checks: git diff --check origin/main...HEAD, py_compile with PYTHONPYCACHEPREFIX, python3 tests/dm-only-strip-preserves-prose.test.py, python3 tests/result-markers.test.py, python3 tests/result-markers-golden.test.py, python3 tests/bridge-marker-no-leak.test.py, python3 tests/ag2-sparrow-drift.test.py, and bash tests/review-checks.test.sh passed. Current-head GitHub checks are green except tsc + tests (clean install), which is still running, so I am leaving this as a review comment rather than a formal approval until that gate finishes.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 215d5add4301f262bb5f488dfad373c8be06b66b. This is a merge-only refresh; the full seven-file topic diff retains the previously approved stable patch ID (f74e9fd35472fc427561432944bd4724cc1e0e53). Detection remains fail-safe anywhere in the body, while stripping is standalone-line-only in both Python copies and task-bridge, preserving owner prose without weakening redirect suppression.

Reviewer-run tests/result-markers.test.py passes 34/34 and tests/dm-only-strip-preserves-prose.test.py passes 15/15, including executed Node/Python parity controls. Diff and REVIEW.md path checks pass; all exact-head GitHub checks and CLA are green. The worst-case privacy regression remains bounded because inline mentions still trigger dm-only routing even though their text is preserved.

Verdict: approved; code-ready on this head.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 5fe06765e79e6363984116ff10ea587df285bfc9.

No new code blocker in this merge-only refresh. The seven-file topic diff still preserves the intended privacy split: [dm-only] detection remains anywhere/fail-safe, while stripping is standalone-line-only in both Python copies and the task/voice TypeScript consumer, so inline owner prose survives without allowing redirects.

Reviewer-run checks passed:

  • result markers 34/34
  • dm-only prose/Node parity 15/15
  • golden corpus 13/13
  • bridge marker/no-leak and Sparrow drift suites
  • Python 3.9 compilation
  • diff hygiene and REVIEW.md path gate

Diff coverage, static checks, smoke, and CLA are green. The exact-head clean-install job is still pending, so this is code-clean but not yet merge-ready; I am leaving a COMMENT verdict instead of over-claiming formal approval before that required gate completes.

@john-the-dev
john-the-dev merged commit de90bff into main Aug 1, 2026
19 checks passed
@john-the-dev
john-the-dev deleted the fix/dm-only-strip-must-not-mangle-prose branch August 1, 2026 18:46
john-the-dev added a commit that referenced this pull request Aug 1, 2026
)

`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

Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants