feat(signal): add timestamp edits and opt-in tool progress - #34561
feat(signal): add timestamp edits and opt-in tool progress#34561dorukardahan wants to merge 15 commits into
Conversation
|
Hey @dorukardahan — glad to see this PR. We've been carrying a near-identical patch locally since 5/25 (same 1.
|
|
This is useful work and it lines up with part of #39043. One caution before merging this with Today that flag is used as more than “explicit edits are possible”. It also affects streaming/progress behavior in the gateway. In current So I think the safer shape is to decouple two capabilities:
The timestamp plumbing itself looks valuable, but flipping Signal directly to streaming-edit eligible may introduce UX/rate/chaining issues. @vb3's note above about chained edit timestamps is one concrete example of why this probably deserves a staged path: first expose stable ids and explicit edit support, then enable streaming/progress edits only after the gateway can track the evolving edit timestamp safely. AI-assisted note: this comment was drafted with AI assistance and reviewed before posting. |
|
Follow-up on the streaming/editing concern:
|
|
Thanks @dorukardahan — the fresh-timestamp return on I see three edit cadences here, not two: explicit one-off edits, the tool-progress bubble (batched, throttled to ~1.5s), and token-by-token response streaming (many edits/sec). And on that path Signal breaks after the first edit. Signal edits are timestamp-addressed: each successful edit mints a new timestamp the next edit has to target. But the loop pins The fix should be Signal-scoped, not a blanket "adopt the returned id." Slack echoes the same ts, Telegram/Discord keep the same id on the normal in-place edit, and Matrix returns a fresh replacement event but its later edits must still target the original — so pin-to-original is already correct for all of them; only Signal needs the returned ts to become the next target. So either a small capability ("returned edit id is the next edit target," set by Signal only), or have Separately: whether Signal should show a live tool-progress bubble at all is a fair UX call — but it's distinct from token streaming (a throttled bubble is much milder than per-token edits), so I'd gate it on tool-progress's own capability rather than reuse As-is it's the in-between state: tool-progress runs on Signal but breaks at edit #2. Happy to send a follow-up commit either way. Drafted with AI assistance, reviewed by @vb3. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for separating explicit Signal edits from automatic streaming and progress cadence. The Signal timestamp plumbing addresses a real current-main gap, but two delivery-contract issues need resolution.
Problems
gateway/platforms/signal.py:1158-1160returns the fresh Signal timestamp, but the long-running heartbeat loop reuses_heartbeat_msg_idon every edit (gateway/run.py:19417-19436) and only updates it after a fallback send. A second Signal heartbeat therefore retains the old edit target.gateway/stream_consumer.py:1669adopts every changed edit result id globally. Matrix returns a new replacement event id (plugins/platforms/matrix/adapter.py:1741-1746) while using the supplied id as them.replacetarget (plugins/platforms/matrix/adapter.py:1735-1738); the base adapter contract does not establish that all fresh result ids are valid next-edit targets.
Suggested changes
- Scope next-target propagation to an explicit Signal capability or keep the mapping in
SignalAdapter; cover two successive Signal edits, including the heartbeat caller. - Remove or capability-gate the generic stream-consumer id replacement and add a replacement-event regression.
Automated hermes-sweeper review.
|
CI note for head |
|
Refresh complete on head
Verification for this head:
The two existing review threads remain resolved. GitHub currently reports the PR AI-assisted refresh; reviewed and test-verified before push. |
Related to #39043: this implements the Signal edit/progress slice with timestamp chaining and a separate streaming gate; the open issue remains the broader capability spec for maintainer review. |
|
Follow-up refresh on head A final read-only review found that proxy/SSE mode could still construct a Regression coverage drives the real proxy SSE assembly path with a timestamp-returning Signal-like adapter and asserts:
Verification for the current head:
The branch included current upstream AI-assisted refresh; reviewed and test-verified before push. |
|
One scope clarification from a fresh current-head + signal-cli v0.14.6 audit: At The edit/revision-chain implementation itself still looks correct and all current review threads are resolved. I suggest narrowing the PR summary to “text sends/edits” and either:
This matters for later remote-delete, ephemeral TTL, and durable references: those operations still cannot target media messages even after this PR as currently written. AI-assisted: Codex checked the current PR head and the tagged signal-cli v0.14.6 source; the finding and comment were reviewed before posting. |
# Conflicts: # tests/gateway/test_run_progress_topics.py # tests/gateway/test_stream_consumer.py
|
Refreshed onto current
|
|
Refreshed once more onto current Candidate
|
…t-message-20260601 # Conflicts: # gateway/run.py # gateway/stream_consumer.py # tests/gateway/test_run_progress_topics.py # tests/gateway/test_signal.py # tests/gateway/test_signal_format.py
Summary
send+editTimestamp/verboseordisplay.platforms.signal.tool_progress)tool_progressdefault atoffWhy
Modern Signal and signal-cli support editing already-sent text messages. Hermes already had a generic edit path, but the Signal adapter discarded text-send timestamps and advertised no edit capability. As a result, explicit text edits were unavailable and gateway tool progress was suppressed even when a user opted in.
Signal's edit contract is also different from ordinary message-ID platforms: each edit creates a fresh timestamp, and the next edit must target that timestamp. Reusing only the original
editTimestampcan report success while later client updates are silently ignored.This PR deliberately does not add attachment/media edit semantics. Signal media delivery remains on its existing non-edit path.
Implementation
SignalAdapter.send()exposes the signal-cli response timestamp for text messages asSendResult.message_idSignalAdapter.edit_message()edits text viaeditTimestamp, preserves Signal-native text formatting, supports DM and group routing, and fails closed when signal-cli does not return the fresh next timestampEDIT_RESULT_ID_IS_NEXT_TARGETmakes timestamp chaining explicit instead of inferring it from every adapter'sSendResultSUPPORTS_STREAMING_EDITSandSUPPORTS_PROGRESS_EDITSseparate high-frequency response streaming from lower-frequency, user-enabled text progress editsSignal UX
Default behavior is unchanged: Signal remains quiet and sends only the final response.
Opt in with:
After that,
/verbosecycles Signal's progress mode. Text tool progress is accumulated into one edited message; token streaming remains disabled.Verification
Current-main refreshed head:
Focused coverage includes:
editTimestamppayloads and native text formattingfinalizesignature compatibilityAdditional checks:
ruff checkon every changed Python file: passedpy_compileon every changed Python file: passedgit diff --check: passedAdvances #39043.