Extract shared call protocol, crypto, and key package helpers#486
Merged
justinmoon merged 4 commits intomasterfrom Mar 8, 2026
Merged
Extract shared call protocol, crypto, and key package helpers#486justinmoon merged 4 commits intomasterfrom
justinmoon merged 4 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR extracts and consolidates call signaling and key package management logic into a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/pikachat-sidecar/src/daemon.rs (1)
751-762:⚠️ Potential issue | 🟠 MajorRedact call payloads before logging parse failures.
These warnings can dump
relay_authfrom invite/accept bodies into logs. A malformed signal should not turn a bearer capability into log data.Also applies to: 805-810
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pikachat-sidecar/src/daemon.rs` around lines 751 - 762, The parse-failure warning for CallSignalEnvelopeCompat logs the raw content which can leak sensitive fields like relay_auth; update the error handling in the serde_json::from_str::<CallSignalEnvelopeCompat>(content) Err branch (and the analogous block handling call signals later) to redact the call payload before logging: detect when content contains "pika.call", "call.invite", or "call.accept" and replace or mask the body/details (e.g., remove JSON fields that may contain credentials or replace with a fixed "[REDACTED_CALL_PAYLOAD]") and then log the sanitized string in the warn! call instead of the raw content so parse errors are still surfaced without leaking bearer capabilities.rust/src/core/call_control.rs (1)
823-833:⚠️ Potential issue | 🟠 MajorCore dropped the compatibility parsing that the sidecar still keeps.
crates/pikachat-sidecar/src/daemon.rsstill unwraps double-encoded payloads and nestedcontent/rumor.contentenvelopes before calling the shared parser. Calling the strict parser directly here means those same call signals will be silently ignored in core.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/core/call_control.rs` around lines 823 - 833, maybe_parse_call_signal currently calls the strict parse_call_signal directly and thus drops the sidecar's compatibility handling for double-encoded payloads and nested content/rumor.content envelopes; update maybe_parse_call_signal to first perform the same compatibility unwrapping the sidecar uses (try decoding double-encoded JSON, unwrap nested "content" or "rumor.content" fields) and only then pass the resulting string to parse_call_signal (or create a small helper like compat_parse_call_signal that encapsulates the unwrap logic and calls parse_call_signal). Ensure you still keep the early-return check against self.session.pubkey (the existing my_pubkey check) and use the same error-tolerant behavior as the sidecar unwrap logic so legacy call signals are not silently ignored.
🧹 Nitpick comments (4)
crates/pika-marmot-runtime/src/relay.rs (1)
66-68: Consider removing the trivial wrapper.
normalize_fetched_key_package_for_mdkis a pass-through tonormalize_peer_key_package_event_for_mdk. You could call the imported function directly infetch_latest_key_package_for_mdkunless you anticipate adding relay-specific normalization logic here.♻️ Proposed simplification
pub async fn fetch_latest_key_package_for_mdk( client: &Client, author: &PublicKey, relay_urls: &[RelayUrl], timeout: Duration, ) -> Result<Event> { let event = fetch_latest_key_package(client, author, relay_urls, timeout).await?; - Ok(normalize_fetched_key_package_for_mdk(&event)) + Ok(normalize_peer_key_package_event_for_mdk(&event)) } - -fn normalize_fetched_key_package_for_mdk(event: &Event) -> Event { - normalize_peer_key_package_event_for_mdk(event) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-marmot-runtime/src/relay.rs` around lines 66 - 68, The function normalize_fetched_key_package_for_mdk is a trivial passthrough to normalize_peer_key_package_event_for_mdk; remove the wrapper and update callers (notably fetch_latest_key_package_for_mdk) to call normalize_peer_key_package_event_for_mdk directly, or if you want to keep a relay-specific hook, keep the wrapper but add relay-specific logic—ensure no remaining references to normalize_fetched_key_package_for_mdk remain after the change.crates/pika-marmot-runtime/src/call.rs (1)
119-125: Consider documenting the timestamp overflow behavior.The cast
as_millis() as i64could theoretically overflow for dates beyond ~292 million years from Unix epoch. While practically safe, a brief comment noting this is intentionally ignored for call signaling purposes would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-marmot-runtime/src/call.rs` around lines 119 - 125, In function now_millis(), the conversion `.as_millis() as i64` can theoretically overflow for timestamps far beyond practical ranges; add a short inline comment or doc comment above now_millis() stating that potential overflow is intentionally ignored (dates ~292 million years out) because the value is only used for call signaling and the cast is safe for all expected use cases. Reference the now_millis() function and its use of SystemTime::now(), UNIX_EPOCH, and as_millis() when adding the explanatory note.cli/src/harness.rs (1)
1609-1621: Prefer the shared CLI relay helper over a second local wrapper.The rest of the CLI already goes through
relay_util::fetch_latest_key_package_for_mdk; keeping another wrapper here duplicates the timeout/context policy and makes future drift easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/harness.rs` around lines 1609 - 1621, The local wrapper async fn fetch_latest_key_package_for_mdk duplicates timeout/context behavior; replace its usage with the shared helper relay_util::fetch_latest_key_package_for_mdk instead of calling pika_marmot_runtime::relay::fetch_latest_key_package_for_mdk here, remove this local wrapper if no other callers remain, and update imports/usages to import relay_util::fetch_latest_key_package_for_mdk (or fully qualify it) so the CLI consistently uses the shared relay helper.cli/src/main.rs (1)
1330-1358: Extract the peer key-package fallback into one helper.These two blocks now duplicate the same primary fetch, default-relay fallback, timeout, and error text. Pulling that into a shared helper will keep
sendandsend-hypernotefrom drifting the next time the relay policy changes.Also applies to: 1531-1558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/main.rs` around lines 1330 - 1358, Extract the duplicated fetch-and-fallback logic into a single helper (e.g. get_peer_key_package_with_fallback) that encapsulates calling relay_util::fetch_latest_key_package_for_mdk with the timeout, checking cli.kp_relay.is_empty() and comparing kp_relays != relays to decide the fallback to relays, and producing the same contextualized errors currently built with primary_err; replace the duplicated blocks that set peer_kp (including the occurrence around send and the other occurrence at the send-hypernote site) with a call to this helper, passing c, peer_pubkey, kp_relays, relays, cli.kp_relay, and Duration::from_secs(10). Ensure the helper returns the key package or an appropriate anyhow::Error with the existing context strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/harness.rs`:
- Around line 339-342: The current call to fetch_latest_key_package_for_mdk may
return a normalized copy which makes the subsequent wire-ID assertion (comparing
fetched_b_kp.id to b_kp.id) flaky; replace the call to
fetch_latest_key_package_for_mdk with the raw fetch path (e.g., the
non-normalizing fetch function used elsewhere such as fetch_latest_key_package)
so fetched_b_kp is the original relay event, or alternatively apply the same
MDK-normalization to b_kp before doing the comparison; update the invocation
that creates fetched_b_kp and ensure the comparison uses matching forms.
In `@crates/pika-marmot-runtime/src/key_package.rs`:
- Around line 48-51: The code uses s.len().is_multiple_of(2) (inside the
content_is_hex block) which requires Rust 1.87+; update the crate to declare
rust-version = "1.87" in Cargo.toml or replace the call with a compatible check
such as s.len() % 2 == 0 to support the current edition; modify either the
Cargo.toml rust-version or the expression in the content_is_hex computation
accordingly so the crate compiles on Rust 1.85/1.86.
In `@crates/pikachat-sidecar/src/daemon.rs`:
- Around line 872-884: The code derives media keys from session.tracks.first()
via call_primary_track_name(session) which can pick a non-audio track; instead
determine the audio track using call_audio_track_spec(session) (or the helper
that returns the chosen audio track) and pass that result into
derive_shared_call_media_crypto_context. Replace the
call_primary_track_name(session) usage with the actual audio track obtained from
call_audio_track_spec(session) (handle its Result/Option the same way as
call_primary_track_name currently is handled) and propagate errors with
.map_err(anyhow::Error::msg) so tx_keys/rx_keys are derived from the real audio
track.
In `@rust/src/core/call_control.rs`:
- Around line 149-165: handle_incoming_call_signal currently hardcodes
"audio0"/"video0" when building CallCryptoDeriveContext and calling
derive_shared_call_media_crypto_context, which allows mismatched negotiated
track names to pass signaling but fail at media runtime; update the handler to
inspect session.tracks to (a) find the actual negotiated audio and video track
names (e.g., by searching session.tracks for the audio/video types or configured
role) and pass those names into derive_shared_call_media_crypto_context instead
of the literal "audio0"/"video0", or (b) if your protocol requires fixed names,
validate session.tracks up front and return an error/reject the call when the
expected names are not present; adjust CallCryptoDeriveContext/derive call sites
to use the discovered/validated track name variables rather than hardcoded
strings.
In `@todos/pika-core-final.md`:
- Line 345: Replace the typo "revertable" with the correct spelling "revertible"
in the sentence "keep PRs independently reviewable and revertable" so it reads
"keep PRs independently reviewable and revertible".
---
Outside diff comments:
In `@crates/pikachat-sidecar/src/daemon.rs`:
- Around line 751-762: The parse-failure warning for CallSignalEnvelopeCompat
logs the raw content which can leak sensitive fields like relay_auth; update the
error handling in the serde_json::from_str::<CallSignalEnvelopeCompat>(content)
Err branch (and the analogous block handling call signals later) to redact the
call payload before logging: detect when content contains "pika.call",
"call.invite", or "call.accept" and replace or mask the body/details (e.g.,
remove JSON fields that may contain credentials or replace with a fixed
"[REDACTED_CALL_PAYLOAD]") and then log the sanitized string in the warn! call
instead of the raw content so parse errors are still surfaced without leaking
bearer capabilities.
In `@rust/src/core/call_control.rs`:
- Around line 823-833: maybe_parse_call_signal currently calls the strict
parse_call_signal directly and thus drops the sidecar's compatibility handling
for double-encoded payloads and nested content/rumor.content envelopes; update
maybe_parse_call_signal to first perform the same compatibility unwrapping the
sidecar uses (try decoding double-encoded JSON, unwrap nested "content" or
"rumor.content" fields) and only then pass the resulting string to
parse_call_signal (or create a small helper like compat_parse_call_signal that
encapsulates the unwrap logic and calls parse_call_signal). Ensure you still
keep the early-return check against self.session.pubkey (the existing my_pubkey
check) and use the same error-tolerant behavior as the sidecar unwrap logic so
legacy call signals are not silently ignored.
---
Nitpick comments:
In `@cli/src/harness.rs`:
- Around line 1609-1621: The local wrapper async fn
fetch_latest_key_package_for_mdk duplicates timeout/context behavior; replace
its usage with the shared helper relay_util::fetch_latest_key_package_for_mdk
instead of calling pika_marmot_runtime::relay::fetch_latest_key_package_for_mdk
here, remove this local wrapper if no other callers remain, and update
imports/usages to import relay_util::fetch_latest_key_package_for_mdk (or fully
qualify it) so the CLI consistently uses the shared relay helper.
In `@cli/src/main.rs`:
- Around line 1330-1358: Extract the duplicated fetch-and-fallback logic into a
single helper (e.g. get_peer_key_package_with_fallback) that encapsulates
calling relay_util::fetch_latest_key_package_for_mdk with the timeout, checking
cli.kp_relay.is_empty() and comparing kp_relays != relays to decide the fallback
to relays, and producing the same contextualized errors currently built with
primary_err; replace the duplicated blocks that set peer_kp (including the
occurrence around send and the other occurrence at the send-hypernote site) with
a call to this helper, passing c, peer_pubkey, kp_relays, relays, cli.kp_relay,
and Duration::from_secs(10). Ensure the helper returns the key package or an
appropriate anyhow::Error with the existing context strings.
In `@crates/pika-marmot-runtime/src/call.rs`:
- Around line 119-125: In function now_millis(), the conversion `.as_millis() as
i64` can theoretically overflow for timestamps far beyond practical ranges; add
a short inline comment or doc comment above now_millis() stating that potential
overflow is intentionally ignored (dates ~292 million years out) because the
value is only used for call signaling and the cast is safe for all expected use
cases. Reference the now_millis() function and its use of SystemTime::now(),
UNIX_EPOCH, and as_millis() when adding the explanatory note.
In `@crates/pika-marmot-runtime/src/relay.rs`:
- Around line 66-68: The function normalize_fetched_key_package_for_mdk is a
trivial passthrough to normalize_peer_key_package_event_for_mdk; remove the
wrapper and update callers (notably fetch_latest_key_package_for_mdk) to call
normalize_peer_key_package_event_for_mdk directly, or if you want to keep a
relay-specific hook, keep the wrapper but add relay-specific logic—ensure no
remaining references to normalize_fetched_key_package_for_mdk remain after the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94a11666-676a-4a7a-83ce-b37c39d5dda0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
cli/src/agent/session.rscli/src/harness.rscli/src/main.rscli/src/relay_util.rscrates/pika-marmot-runtime/Cargo.tomlcrates/pika-marmot-runtime/src/call.rscrates/pika-marmot-runtime/src/key_package.rscrates/pika-marmot-runtime/src/lib.rscrates/pika-marmot-runtime/src/relay.rscrates/pikachat-sidecar/src/daemon.rsrust/Cargo.tomlrust/src/core/call_control.rsrust/src/core/call_runtime.rsrust/src/core/interop.rstodos/pika-core-final.md
Comment on lines
+339
to
+342
| let fetched_b_kp = | ||
| fetch_latest_key_package_for_mdk(&a_client, &b_keys.public_key(), relay_url.clone()) | ||
| .await | ||
| .context("fetch B keypackage")?; |
There was a problem hiding this comment.
Don't use the MDK-normalizing fetcher for the wire-ID assertion.
fetch_latest_key_package_for_mdk can return a normalized copy of the relay event. The fetched_b_kp.id != b_kp.id check on Line 343 then becomes flaky and can fail for a valid legacy/interop key package. Keep the raw fetch path here, or normalize b_kp before comparing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/harness.rs` around lines 339 - 342, The current call to
fetch_latest_key_package_for_mdk may return a normalized copy which makes the
subsequent wire-ID assertion (comparing fetched_b_kp.id to b_kp.id) flaky;
replace the call to fetch_latest_key_package_for_mdk with the raw fetch path
(e.g., the non-normalizing fetch function used elsewhere such as
fetch_latest_key_package) so fetched_b_kp is the original relay event, or
alternatively apply the same MDK-normalization to b_kp before doing the
comparison; update the invocation that creates fetched_b_kp and ensure the
comparison uses matching forms.
Comment on lines
+48
to
+51
| let content_is_hex = { | ||
| let s = out.content.trim(); | ||
| !s.is_empty() && s.len().is_multiple_of(2) && s.bytes().all(|b| b.is_ascii_hexdigit()) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When was is_multiple_of stabilized in Rust?
💡 Result:
is_multiple_of (for the primitive unsigned integer types, behind #![feature(unsigned_is_multiple_of)]) was stabilized in Rust 1.87.0, released May 15, 2025. [1] [2]
🏁 Script executed:
# Find and examine Cargo.toml for the crate
find . -name "Cargo.toml" | grep -E "(pika-marmot|root)" | head -5Repository: sledtools/pika
Length of output: 97
🏁 Script executed:
# Check for rust-toolchain or similar files
find . -name "rust-toolchain*" -o -name ".rust-version" | head -10Repository: sledtools/pika
Length of output: 40
🏁 Script executed:
# Look at the Cargo.toml for pika-marmot-runtime
cat crates/pika-marmot-runtime/Cargo.tomlRepository: sledtools/pika
Length of output: 643
🏁 Script executed:
# Check root Cargo.toml for workspace rust-version
head -50 Cargo.tomlRepository: sledtools/pika
Length of output: 1742
🏁 Script executed:
# View the actual file to verify the code and context
cat -n crates/pika-marmot-runtime/src/key_package.rs | sed -n '40,60p'Repository: sledtools/pika
Length of output: 1092
🌐 Web query:
Rust 2024 edition minimum Rust version requirement
💡 Result:
Rust 2024 Edition requires Rust 1.85.0 or newer (stable), since the edition was stabilized in Rust 1.85.0 (2025-02-20). [1][2]
If you set it in Cargo.toml, use:
edition = "2024"rust-version = "1.85"(minimum toolchain for the edition) [2]
Sources:
[1] Rust Release Notes (Rust 1.85.0: “The 2024 Edition is now stable.”)
[2] Rust Blog: “Rust 2024 in beta channel” (recommends rust-version = "1.85")
Set explicit rust-version or use compatible alternative.
The is_multiple_of method requires Rust 1.87.0+ (stabilized May 15, 2025), but the crate's edition = "2024" only guarantees Rust 1.85.0+. This will cause compilation failures on Rust 1.85.x and 1.86.x. Either add rust-version = "1.87" to Cargo.toml or use s.len() % 2 == 0 for compatibility with the stated edition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pika-marmot-runtime/src/key_package.rs` around lines 48 - 51, The code
uses s.len().is_multiple_of(2) (inside the content_is_hex block) which requires
Rust 1.87+; update the crate to declare rust-version = "1.87" in Cargo.toml or
replace the call with a compatible check such as s.len() % 2 == 0 to support the
current edition; modify either the Cargo.toml rust-version or the expression in
the content_is_hex computation accordingly so the crate compiles on Rust
1.85/1.86.
Comment on lines
+872
to
+884
| let primary_track = call_primary_track_name(session)?; | ||
| let derive_ctx = CallCryptoDeriveContext { | ||
| mdk, | ||
| &mls_group_id, | ||
| DEFAULT_SCHEME_VERSION, | ||
| &root_hash, | ||
| "application/pika-call", | ||
| &root_filename, | ||
| ) | ||
| .map_err(|e| anyhow!("derive media group root failed: {e}"))?; | ||
|
|
||
| Ok(CallMediaCryptoContext { | ||
| tx_keys: FrameKeyMaterial::from_base_key( | ||
| tx_base, | ||
| key_id_for_sender(local_pubkey_hex.as_bytes()), | ||
| group.epoch, | ||
| generation, | ||
| track, | ||
| group_root, | ||
| ), | ||
| rx_keys: FrameKeyMaterial::from_base_key( | ||
| rx_base, | ||
| key_id_for_sender(peer_pubkey_hex.as_bytes()), | ||
| group.epoch, | ||
| generation, | ||
| track, | ||
| group_root, | ||
| ), | ||
| local_participant_label: opaque_participant_label(&group_root, local_pubkey_hex.as_bytes()), | ||
| peer_participant_label: opaque_participant_label(&group_root, peer_pubkey_hex.as_bytes()), | ||
| }) | ||
| mls_group_id: &mls_group_id, | ||
| group_epoch: group.epoch, | ||
| call_id, | ||
| session, | ||
| local_pubkey_hex, | ||
| peer_pubkey_hex, | ||
| }; | ||
|
|
||
| derive_shared_call_media_crypto_context(&derive_ctx, primary_track, None) | ||
| .map_err(anyhow::Error::msg) |
There was a problem hiding this comment.
Derive keys from the actual audio track, not the first track.
Audio mode is chosen via call_audio_track_spec(session), but this path derives tx_keys/rx_keys from session.tracks.first(). A session with a non-audio track first will accept and then fail every audio encrypt/decrypt.
Suggested fix
- let primary_track = call_primary_track_name(session)?;
+ let primary_track = if let Some(track) = call_audio_track_spec(session) {
+ track.name.as_str()
+ } else {
+ call_primary_track_name(session)?
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikachat-sidecar/src/daemon.rs` around lines 872 - 884, The code
derives media keys from session.tracks.first() via
call_primary_track_name(session) which can pick a non-audio track; instead
determine the audio track using call_audio_track_spec(session) (or the helper
that returns the chosen audio track) and pass that result into
derive_shared_call_media_crypto_context. Replace the
call_primary_track_name(session) usage with the actual audio track obtained from
call_audio_track_spec(session) (handle its Result/Option the same way as
call_primary_track_name currently is handled) and propagate errors with
.map_err(anyhow::Error::msg) so tx_keys/rx_keys are derived from the real audio
track.
Comment on lines
+149
to
165
| let video_track = session | ||
| .tracks | ||
| .iter() | ||
| .any(|t| t.name == "video0") | ||
| .then_some("video0"); | ||
| let derive_ctx = CallCryptoDeriveContext { | ||
| mdk: &sess.mdk, | ||
| mls_group_id: &group_entry.mls_group_id, | ||
| group_epoch: group.epoch, | ||
| call_id, | ||
| session, | ||
| local_pubkey_hex, | ||
| peer_pubkey_hex, | ||
| "audio0", | ||
| )?; | ||
|
|
||
| let has_video = session.tracks.iter().any(|t| t.name == "video0"); | ||
| let (video_tx_keys, video_rx_keys) = if has_video { | ||
| let (vtx, vrx, _) = self.derive_track_keys( | ||
| chat_id, | ||
| call_id, | ||
| session, | ||
| local_pubkey_hex, | ||
| peer_pubkey_hex, | ||
| "video0", | ||
| )?; | ||
| (Some(vtx), Some(vrx)) | ||
| } else { | ||
| (None, None) | ||
| }; | ||
|
|
||
| Ok(super::call_runtime::CallMediaCryptoContext { | ||
| tx_keys, | ||
| rx_keys, | ||
| video_tx_keys, | ||
| video_rx_keys, | ||
| local_participant_label: opaque_participant_label( | ||
| &group_root, | ||
| local_pubkey_hex.as_bytes(), | ||
| ), | ||
| peer_participant_label: opaque_participant_label( | ||
| &group_root, | ||
| peer_pubkey_hex.as_bytes(), | ||
| ), | ||
| }) | ||
| derive_shared_call_media_crypto_context(&derive_ctx, "audio0", video_track) | ||
| } |
There was a problem hiding this comment.
Either honor negotiated track names or reject them up front.
The shared helper now takes explicit track names, but this still hardcodes "audio0" / "video0". handle_incoming_call_signal accepts arbitrary session.tracks, and the media runtime still publishes/subscribes fixed names, so a peer using different names can clear signaling/auth and only fail once media starts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/core/call_control.rs` around lines 149 - 165,
handle_incoming_call_signal currently hardcodes "audio0"/"video0" when building
CallCryptoDeriveContext and calling derive_shared_call_media_crypto_context,
which allows mismatched negotiated track names to pass signaling but fail at
media runtime; update the handler to inspect session.tracks to (a) find the
actual negotiated audio and video track names (e.g., by searching session.tracks
for the audio/video types or configured role) and pass those names into
derive_shared_call_media_crypto_context instead of the literal
"audio0"/"video0", or (b) if your protocol requires fixed names, validate
session.tracks up front and return an error/reject the call when the expected
names are not present; adjust CallCryptoDeriveContext/derive call sites to use
the discovered/validated track name variables rather than hardcoded strings.
| - one subsystem per PR | ||
| - app consumes the extracted code first when practical | ||
| - daemon follows onto it after the pattern is proven | ||
| - keep PRs independently reviewable and revertable |
There was a problem hiding this comment.
Minor typo: "revertable" → "revertible".
The standard spelling is "revertible" (or alternatively "reversible").
📝 Proposed fix
-- keep PRs independently reviewable and revertable
+- keep PRs independently reviewable and revertible📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - keep PRs independently reviewable and revertable | |
| - keep PRs independently reviewable and revertible |
🧰 Tools
🪛 LanguageTool
[grammar] ~345-~345: Ensure spelling is correct
Context: ...- keep PRs independently reviewable and revertable ## Success Criteria We are succeeding if: ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@todos/pika-core-final.md` at line 345, Replace the typo "revertable" with the
correct spelling "revertible" in the sentence "keep PRs independently reviewable
and revertable" so it reads "keep PRs independently reviewable and revertible".
This was referenced Mar 8, 2026
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
pika-marmot-runtimeso both the sidecar daemon and core can reuse itpika-marmot-runtime, shrinkingcall_control.rsanddaemon.rssignificantlytodos/pika-core-final.mdNet effect: ~1750 lines added, ~875 removed. The bulk of the diff is moving duplicated MLS/call logic into the shared
pika-marmot-runtimecrate.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores