chore: finish channel migration — repair build, test gates, docs#10
Open
YellowSnnowmann wants to merge 6 commits into
Open
chore: finish channel migration — repair build, test gates, docs#10YellowSnnowmann wants to merge 6 commits into
YellowSnnowmann wants to merge 6 commits into
Conversation
The dependabot bump to hmac 0.13 (PR tinyhumansai#4) requires digest 0.11, but the sha2/sha1 hashes remain on 0.10 (digest 0.10), so Hmac<Sha256>/Hmac<Sha1> no longer satisfy their trait bounds and every call site still uses the 0.12 new_from_slice API. This left main red (CI failure on the current HEAD) and unbuildable at relay/auth.rs, providers/linq.rs, and providers/yuanbao/{sign,cos}.rs. Revert to hmac 0.12 to match the hash stack the code targets and prune the orphaned 0.13 entry from the lockfile. Moving the whole SHA stack to digest 0.11 is left as a separate coordinated bump.
The negotiation suite only exercised Text/Media/ReplyTo/Thread. Add exhaustive coverage over every DurableFinalDeliveryCapability variant (downgrade-when-unsupported and preserve-when-supported), an advertised-as-false case, the Disabled short-circuit, multi-missing reporting, and the full OutboundPayload->requirement mapping.
Nothing tied ChannelAuthMode to the connect controller schema, whose authMode description hardcodes the wire-name list; a new auth mode could ship without appearing in the schema and only fail at runtime. Add a parity test with an exhaustive match (new variants fail to compile until registered) that also checks Display/FromStr round-trips and that the connect schema description advertises each wire name.
The relay-websocket transport had only URL/token/config unit tests; real framed I/O, upgrade authorization, and reconnect went through the in-memory mock. Add in-process tokio-tungstenite loopback tests that dial the real WebSocketRelayIo and assert newline-delimited frame sequencing (batched connector frames decoded in order plus a gateway frame echoed back), upgrade-token accept/reject at the handshake, and reconnect through WebSocketRelayDialer.
The execution plan (2026-07-04) and porting.md predate the provider port: 16 providers now live in src/providers/ behind the src/host/ service boundary, and OpenHuman consumes them as re-export shims. Correct porting.md's 'Not Yet Ported' section, add a dated addendum to the execution plan, and add the remaining-work plan as the source of truth for the finishing slices.
📝 WalkthroughWalkthroughThe changes update migration documentation, pin ChangesMigration validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
CI runs clippy on the current stable toolchain (1.97), which flags two pre-existing lints that older clippy did not: a question_mark rewrite in telegram/channel_recv.rs and a redundant reference in a format! argument in yuanbao/sign.rs. Apply clippy's own suggestions (an ? early-return and dropping the &) so `cargo clippy --all-targets --all-features -- -D warnings` is clean on stable. No behavior change.
YellowSnnowmann
marked this pull request as ready for review
July 17, 2026 12:36
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/delivery/test.rs (1)
657-666: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete capability map.
Checking only the expected entry allows accidental extra requirements to pass, potentially causing unnecessary
BestEffortdowngrades.Proposed assertion
for (payload, expected) in cases { let requirements = required_durable_final_capabilities(&intent(payload.clone())); + let mut expected_requirements = + crate::channel::DurableFinalDeliveryRequirementMap::new(); + expected_requirements.insert(expected, true); assert_eq!( - requirements.get(&expected), - Some(&true), + requirements, + expected_requirements, "{payload:?} should require {expected:?}" ); - assert!(!requirements.contains_key(&ReplyTo)); - assert!(!requirements.contains_key(&Thread)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/delivery/test.rs` around lines 657 - 666, Update the assertions in the required_durable_final_capabilities test loop to compare the complete returned capability map against the expected map for each case, rather than checking only the expected entry. Preserve the existing validation that ReplyTo and Thread remain absent, and ensure unexpected capability requirements cause the test to fail.src/relay/test.rs (1)
853-913: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd deadlines to the loopback tests.
The unbounded
accept,next, connection, and server-join awaits can hang CI indefinitely after a protocol or connection-close regression. Give each scenario a test-level timeout and abort its server task on timeout.Also applies to: 915-936, 938-986
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/relay/test.rs` around lines 853 - 913, Wrap the loopback test scenarios, including roundtrips_frames_in_order_over_a_real_socket and the additional tests around them, in test-level timeouts covering connection, accept, websocket next, and server join awaits. On timeout, abort the spawned server task before failing the test, while preserving the existing assertions and protocol behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/porting.md`:
- Around line 51-75: Update the later current-status paragraph in
docs/porting.md to remove stale claims that provider wire adapters remain
pending, aligning it with the resolved migration described near the provider
portability ladder. Keep only non-relay session-key adoption as pending, while
preserving the existing WhatsApp Web durable-session backend debt if it is still
accurate.
In `@docs/spec/tinychannels-migration-remaining-plan.md`:
- Around line 23-27: Correct the provider inventory/count discrepancy by using
15 providers, matching the enumerated list. Update the source-of-truth summary
in docs/spec/tinychannels-migration-remaining-plan.md, then align the
corresponding progress summaries in docs/porting.md and
docs/spec/tinychannels-execution-plan.md; do not add a provider.
- Around line 56-65: Update the source-of-truth migration plan to mark G0, G1,
G2, G3, and G4 as completed, removing G0’s Blocker status from the ranked table.
Revise the corresponding detailed sections, sequencing guidance, and G1 status
so they describe the post-merge state, while leaving unresolved slices G5, G6,
and G7 as remaining work.
---
Nitpick comments:
In `@src/delivery/test.rs`:
- Around line 657-666: Update the assertions in the
required_durable_final_capabilities test loop to compare the complete returned
capability map against the expected map for each case, rather than checking only
the expected entry. Preserve the existing validation that ReplyTo and Thread
remain absent, and ensure unexpected capability requirements cause the test to
fail.
In `@src/relay/test.rs`:
- Around line 853-913: Wrap the loopback test scenarios, including
roundtrips_frames_in_order_over_a_real_socket and the additional tests around
them, in test-level timeouts covering connection, accept, websocket next, and
server join awaits. On timeout, abort the spawned server task before failing the
test, while preserving the existing assertions and protocol behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cd1e675-ca3a-49f5-9518-65d8b1d3051f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomldocs/porting.mddocs/spec/tinychannels-execution-plan.mddocs/spec/tinychannels-migration-remaining-plan.mdsrc/controllers/definitions_tests.rssrc/delivery/test.rssrc/providers/telegram/channel_recv.rssrc/providers/yuanbao/sign.rssrc/relay/test.rs
Comment on lines
+51
to
+75
| Update 2026-07-17: the provider wire implementations have since been ported. | ||
| `src/providers/` now owns dingtalk, discord, email, imessage, irc, lark, linq, | ||
| mattermost, qq, signal, slack, telegram, whatsapp, whatsapp_web, and yuanbao. | ||
| The app-side dependencies were reduced to a host-owned service boundary | ||
| (`src/host/`: HTTP proxy client, approval, voice/STT, pairing, and | ||
| conversation-memory contracts) that providers call instead of importing | ||
| OpenHuman internals. In OpenHuman the corresponding `channels/providers/*.rs` | ||
| files are now one-line re-exports of the crate providers; Telegram keeps only | ||
| its host-coupled glue (`remote_control`, `bus`, `approval_surface`). | ||
|
|
||
| Per the 2026-07-04 audit, provider portability falls into a ladder: | ||
| The original 2026-07-04 portability ladder, now resolved: | ||
|
|
||
| - Self-contained today (no cross-module OpenHuman imports): email, irc, | ||
| yuanbao, cli, imessage, mattermost, qq, dingtalk, presentation. | ||
| - Need only a configured-HTTP-client trait (they import | ||
| `build_runtime_proxy_client`): discord, slack, whatsapp, lark, signal. | ||
| - Need approval, voice/STT, pairing, and conversation-memory traits first: | ||
| telegram. | ||
| - Not porting targets (they are the app-side consumers of this crate): the | ||
| `runtime/` dispatch engine and the `web` provider. | ||
| - Self-contained (no cross-module OpenHuman imports): email, irc, yuanbao, | ||
| imessage, mattermost, qq, dingtalk — ported. | ||
| - Needed only a configured-HTTP-client trait: discord, slack, whatsapp, lark, | ||
| signal — ported via the host proxy-client contract. | ||
| - Needed approval, voice/STT, pairing, and conversation-memory traits: | ||
| telegram — ported via the host service boundary. | ||
| - Not porting targets (app-side consumers of this crate): the `runtime/` | ||
| dispatch engine and the `web` provider — unchanged, they stay in OpenHuman. | ||
|
|
||
| Remaining provider debt: WhatsApp Web still uses `wacore`'s in-memory session | ||
| backend and re-links after restart, pending a rusqlite-backed durable store | ||
| (see the remaining-work plan). The finishing slices for the migration are | ||
| tracked in [spec/tinychannels-migration-remaining-plan.md](spec/tinychannels-migration-remaining-plan.md). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale provider-migration status.
This section now states that provider wire adapters have been ported, but the file’s later current-status paragraph still says they are pending. Update that status text so the porting document has one authoritative migration state; only non-relay session-key adoption should remain pending.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/porting.md` around lines 51 - 75, Update the later current-status
paragraph in docs/porting.md to remove stale claims that provider wire adapters
remain pending, aligning it with the resolved migration described near the
provider portability ladder. Keep only non-relay session-key adoption as
pending, while preserving the existing WhatsApp Web durable-session backend debt
if it is still accurate.
Comment on lines
+23
to
+27
| - **Phase 6 (providers) mostly complete**: 16 providers live under | ||
| `src/providers/` (dingtalk, discord, email, imessage, irc, lark, linq, | ||
| mattermost, qq, signal, slack, telegram, whatsapp, whatsapp_web, yuanbao). | ||
| In OpenHuman the corresponding files are now **1-line re-exports** | ||
| (e.g. `channels/providers/discord.rs` = `pub use tinychannels::providers::discord::*;`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the provider count consistent across all migration documents.
Each location claims 16 providers, while the enumerated inventory contains 15. Either add the missing provider everywhere or change all three summaries to 15.
docs/spec/tinychannels-migration-remaining-plan.md#L23-L27: correct the source-of-truth inventory/count.docs/porting.md#L51-L53: align the porting summary with the corrected inventory.docs/spec/tinychannels-execution-plan.md#L20-L24: align the progress update with the corrected inventory.
📍 Affects 3 files
docs/spec/tinychannels-migration-remaining-plan.md#L23-L27(this comment)docs/porting.md#L51-L53docs/spec/tinychannels-execution-plan.md#L20-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/spec/tinychannels-migration-remaining-plan.md` around lines 23 - 27,
Correct the provider inventory/count discrepancy by using 15 providers, matching
the enumerated list. Update the source-of-truth summary in
docs/spec/tinychannels-migration-remaining-plan.md, then align the corresponding
progress summaries in docs/porting.md and
docs/spec/tinychannels-execution-plan.md; do not add a provider.
Comment on lines
+56
to
+65
| | ID | Slice | Repo | Effort | Class | | ||
| | --- | --- | --- | --- | --- | | ||
| | G0 | Repair red `main` (hmac crypto-stack break) | tinychannels | S | Blocker | | ||
| | G7 | Session-key `scope_id` switchover for non-relay providers (bug 2) | both | M | Correctness | | ||
| | G5 | WhatsApp-Web durable session storage (rusqlite backend) | tinychannels | L | Functional gap | | ||
| | G2 | Delivery-durability negotiation tests (all 13 capability keys) | tinychannels | M | Test gate | | ||
| | G3 | Controller definitions↔schema parity test | tinychannels | M | Guardrail | | ||
| | G4 | Relay WebSocket loopback acceptance tests | tinychannels | M | Phase-5 gate | | ||
| | G6 | Delete OpenHuman-side portable duplicate tests | openhuman | M | Cleanup | | ||
| | G1 | Rewrite stale plan/porting docs to match reality | both | M | Docs | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mark the completed slices as resolved in this source-of-truth plan.
The PR objectives say G0, G2, G3, G4, and the documentation rewrite G1 are completed here, but this plan still presents them as remaining work and keeps G0 labeled a blocker. Update the ranked table, detailed sections, sequencing, and G1 status so the plan reflects the post-merge state.
Also applies to: 67-89, 126-153, 166-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/spec/tinychannels-migration-remaining-plan.md` around lines 56 - 65,
Update the source-of-truth migration plan to mark G0, G1, G2, G3, and G4 as
completed, removing G0’s Blocker status from the ranked table. Revise the
corresponding detailed sections, sequencing guidance, and G1 status so they
describe the post-merge state, while leaving unresolved slices G5, G6, and G7 as
remaining work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Finishing slices for the OpenHuman channel migration into this crate, batched
in dependency order. The migration itself (Phases 0–6) already largely landed;
this closes the critical build break and hardens three acceptance-gate test
holes, then refreshes the stale plan docs.
Commits, in order:
hmac 0.13bump requiresdigest 0.11, but the hash stack is still ondigest 0.10(sha2 0.10,sha1 0.10), soHmac<Sha256>/Hmac<Sha1>failtheir bounds and the call sites still use the 0.12
new_from_sliceAPI. Thisleft
mainred (CI failing on HEAD). Reverts tohmac 0.12and prunes theorphaned
0.13lock entry. This must land first — the rest build on it.the negotiation suite only exercised 4 of 13
DurableFinalDeliveryCapabilityvariants. Adds exhaustive downgrade/preserve coverage, an advertised-as-false
case, the
Disabledshort-circuit, multi-missing reporting, and the fullOutboundPayload→requirement mapping.ChannelAuthModeto theconnectcontroller schema via an exhaustive match(a new variant fails to compile until registered) plus
Display/FromStrround-trip checks, so the enum and the client-facing schema can't drift.
tokio-tungsteniteloopback covering newline-delimited frame sequencing,upgrade-token accept/reject at the handshake, and reconnect through
WebSocketRelayDialer(previously only the in-memory mock was exercised).porting docs predated the provider port; corrects
porting.md, adds a datedaddendum to the execution plan, and adds the remaining-work plan as the
source of truth.
API Or Behavior Changes
None. Commit 1 is a dependency revert; commits 2–4 are tests only; commit 5 is
docs only. No public API or runtime behavior changes.
Tests
Run locally with
cargo +1.96.1(1.96 is required bylibsqlite3-sys 0.38):cargo fmt --checkcargo clippy --all-targets -- -D warningscargo clippy --all-targets --all-features -- -D warningscargo build --all-targetscargo build --all-targets --all-featurescargo test(949 passed)cargo test --all-featurescargo test --features relay-websocket(relay loopback 3/3)Documentation
docs/porting.mdanddocs/spec/tinychannels-execution-plan.mdupdated tomatch the current provider state;
docs/spec/tinychannels-migration-remaining-plan.mdadded as the source of truth for the remaining finishing work (durable
WhatsApp Web session storage and the non-relay
scope_idsession-keyswitchover).
Summary by CodeRabbit
Bug Fixes
Documentation