Skip to content

feat(l2ps): SR-4 WI-A — channel-over-L2PS transport adapter#95

Closed
Shitikyan wants to merge 3 commits into
mainfrom
feat/sr4-wi-a-channel-transport
Closed

feat(l2ps): SR-4 WI-A — channel-over-L2PS transport adapter#95
Shitikyan wants to merge 3 commits into
mainfrom
feat/sr4-wi-a-channel-transport

Conversation

@Shitikyan

Copy link
Copy Markdown
Contributor

First slice of the DACS negotiation layer on top of the merged SR-4 substrate (#94): wire the transport-agnostic ChannelSession onto the L2PS instant-messaging transport.

What

L2PSChannelTransport (src/l2ps/channel/transport.ts) bridges:

  • out: session.sendOutgoing → JSON → AES-256-GCM under the subnet key → peer.send to every member except self
  • in: peer.onMessage → decrypt → parse → reorder buffersession.receiveIncoming in strict sequence order

Why a reorder buffer

ChannelSession.receiveIncoming accepts any sequence strictly greater than the highest seen and advances its counter to it — a gap (seq N+2 before N+1) silently skips the missing messages rather than rejecting. The L2PS transport delivers by timestamp and replays an offline queue, so out-of-order arrival is normal. The adapter hands the session only the next contiguous sequence, buffers ahead-of-gap, drops duplicates/already-applied, and ignores envelopes for other channels on the same subnet.

Tests

Dependencies declared as structural interfaces (ChannelSessionLike, L2PSMessagingPeerLike) so the reorder logic is unit-tested without real keys or a socket. 5/5: in-order, gap-buffer-and-fill, duplicate-drop, foreign-channel-ignore, send/reply interleave.

Scope

Turn-based messaging (DACS negotiation is offer→counter→accept); concurrent same-sequence emission from both parties is out of SR-4 v1 scope. The SDK stays transport-agnostic — this adapter is the opt-in bridge. Next: WI-B negotiate-rfq phase logic (needs DACS-3 §8.4 spec).

Note — includes a pre-existing build fix

First commit (fix(build): drop invalid override on Error.cause) clears a TS4113 that makes bun run build red on main (BroadcastFailedError/TransportError use override on cause, but tsconfig lib es2021 predates ES2022 Error.cause). The pre-commit build hook blocks all commits until it clears, so it's bundled here — happy to split it into its own PR if preferred; it should land regardless of WI-A.

Shitikyan and others added 2 commits June 23, 2026 17:31
`bun run build` is red on main: BroadcastFailedError and TransportError
declare `public override readonly cause`, but the configured lib
(tsconfig `lib: ["es2021"]`, `target: es2020`) predates ES2022's
`Error.cause`, so the base `Error` has no `cause` to override — TS4113.
The pre-commit build hook blocks every commit until this clears.

Drop the `override` modifier; the class still declares `cause` as its
own property (identical runtime behaviour). Pre-existing — landed with
#94/#91, not introduced here.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bridges the transport-agnostic DACS channel layer (ChannelSession,
which only produces/verifies signed envelopes) onto the L2PS
instant-messaging transport (L2PSMessagingPeer, which moves encrypted
bytes between subnet members).

Core is a reorder buffer. ChannelSession.receiveIncoming accepts any
sequence strictly greater than the highest seen and advances its
counter to it — so a gap (seq N+2 before N+1) silently skips the
missing messages rather than rejecting. The L2PS transport delivers by
timestamp and replays an offline queue, so out-of-order arrival is
normal. The adapter hands the session only the next contiguous
sequence and buffers anything ahead of the gap until it fills;
duplicates / already-applied sequences are dropped; envelopes for
other channels sharing the subnet are ignored.

Outgoing: session.sendOutgoing -> JSON -> AES-256-GCM under the subnet
key -> peer.send to every member except self. Incoming: decrypt ->
parse -> buffer -> drain in order -> session.receiveIncoming (still
enforces signature / sender-in-members / channelId, throwing on tamper).

Dependencies are declared as structural interfaces so the reorder
logic is unit-tested without real signing keys or a live socket. 5/5
tests: in-order, gap-buffer-and-fill, duplicate-drop,
foreign-channel-ignore, send/reply interleave.

Scope: turn-based messaging (DACS negotiation is offer->counter->accept);
concurrent same-sequence emission from both parties is out of SR-4 v1
scope. SDK stays transport-agnostic — this adapter is the opt-in bridge.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@Shitikyan, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 36 minutes and 25 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6d581495-b2c1-46f5-84af-e123299a2b8f

📥 Commits

Reviewing files that changed from the base of the PR and between 4e17d6a and d842ee9.

📒 Files selected for processing (6)
  • src/l2ps/channel/index.ts
  • src/l2ps/channel/transport.ts
  • src/tests/l2ps/transport.e2e.test.ts
  • src/tests/l2ps/transport.test.ts
  • src/websdk/BroadcastFailedError.ts
  • src/websdk/TransportError.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sr4-wi-a-channel-transport

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Complements the unit test (which mocks the session to isolate the
reorder buffer) with a full-stack E2E: two real Demos identities, two
real ChannelSessions, two L2PSChannelTransports over an in-process
relay that stands in for the messaging server. Exercises real CCI-key
signing, real cross-party verification (signature + sender∈members +
channelId), real AES-256-GCM under a shared subnet key, and the reorder
buffer under realistic out-of-order delivery.

3 cases:
- full offer → counter → accept round verifies on both ends; both
  transcripts agree on [1,2,3]
- a two-message burst delivered REVERSED (seq2 before seq1) is buffered
  and applied in order
- a tampered frame surfaces via onError and does not advance the session

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces L2PSChannelTransport, a new adapter that wires the transport-agnostic ChannelSession (DACS negotiation layer) onto the L2PS instant-messaging transport, and includes a pre-existing build fix removing invalid override on Error.cause in two error classes.

  • src/l2ps/channel/transport.ts: The core new file; implements AES-256-GCM encrypt/decrypt, a sequence-ordered reorder buffer, and a serialised promise chain for inbound frame processing. Two places pass this.sharedKey.buffer (the raw ArrayBuffer) to crypto.subtle.importKey instead of the Uint8Array itself — if the key is a subview of a larger buffer, the wrong bytes are used as the AES key, silently breaking decrypt on the remote end.
  • Tests: Well-structured with both isolated unit tests (fake session + fake peer) and E2E tests (real identities, real signatures, in-process relay) covering in-order, gap-fill, reorder, duplicate-drop, and tamper-error paths. All test encryption mirrors the adapter's scheme correctly.
  • Build fix: The override removal on Error.cause in BroadcastFailedError and TransportError is correct and uncontroversial.

Confidence Score: 3/5

The adapter should not merge until the sharedKey.buffer call in both encrypt and decrypt is corrected — callers passing a derived key as a typed-array subview will silently negotiate with the wrong AES key, causing GCM authentication failures on the remote end with no clear diagnostic.

The sharedKey.buffer bug in both encrypt and decrypt is a silent crypto correctness issue: it is masked by the tests (which use new Uint8Array(32).fill(...), a dedicated buffer) but will fail at runtime for any key that arrives as a subview of a larger allocation, which is common in KDF/key-derivation workflows. The reorder buffer, promise-chain serialisation, and test coverage are otherwise solid.

src/l2ps/channel/transport.ts — specifically the two importKey calls in encrypt and decrypt

Important Files Changed

Filename Overview
src/l2ps/channel/transport.ts New adapter bridging ChannelSession ↔ L2PSMessagingPeer; contains a crypto correctness bug (sharedKey.buffer ignores typed-array offset/length in both encrypt and decrypt), a silent-deadlock after verification failure in drain(), and an unserialised appliedSeq update in send()
src/l2ps/channel/index.ts Re-exports the new transport types/class; purely additive barrel change
src/tests/l2ps/transport.test.ts Unit tests for reorder buffer using structural fakes; covers in-order, gap-fill, duplicate-drop, foreign-channel, and interleave scenarios — all with correctly matched test encryption
src/tests/l2ps/transport.e2e.test.ts E2E tests using real Demos identities and ChannelSessions over an in-process relay; covers round-trip verification, out-of-order delivery, and tampered-frame error surfacing
src/websdk/BroadcastFailedError.ts Drops invalid override on cause to fix TS4113 under es2021 lib; correct pre-existing build fix
src/websdk/TransportError.ts Same override removal as BroadcastFailedError; correct build fix

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant App
    participant Transport as L2PSChannelTransport
    participant Session as ChannelSession
    participant Peer as L2PSMessagingPeer

    App->>Transport: "send({ type, body })"
    Transport->>Session: sendOutgoing(opts)
    Session-->>Transport: signed ChannelMessage (seq N)
    Transport->>Transport: "appliedSeq = N"
    Transport->>Transport: encrypt(JSON.stringify(signed))
    loop for each recipient
        Transport->>Peer: send(to, encrypted, messageHash)
    end
    Transport-->>App: ChannelMessage

    Note over Peer,Transport: Inbound path (chained via draining promise)
    Peer->>Transport: onMessage(payload)
    Transport->>Transport: draining.then(ingest(payload))
    Transport->>Transport: decrypt(payload.encrypted)
    Transport->>Transport: JSON.parse → ChannelMessage
    alt channelId mismatch
        Transport->>Transport: ignore
    else seq le appliedSeq
        Transport->>Transport: drop (duplicate)
    else "seq = appliedSeq + 1"
        Transport->>Transport: buffer.set(seq, msg) → drain()
        Transport->>Session: receiveIncoming(msg)
        Transport->>Transport: "appliedSeq = seq"
        Transport->>App: onMessage(msg)
    else seq gt appliedSeq + 1 (gap)
        Transport->>Transport: buffer.set(seq, msg) — held until gap fills
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant App
    participant Transport as L2PSChannelTransport
    participant Session as ChannelSession
    participant Peer as L2PSMessagingPeer

    App->>Transport: "send({ type, body })"
    Transport->>Session: sendOutgoing(opts)
    Session-->>Transport: signed ChannelMessage (seq N)
    Transport->>Transport: "appliedSeq = N"
    Transport->>Transport: encrypt(JSON.stringify(signed))
    loop for each recipient
        Transport->>Peer: send(to, encrypted, messageHash)
    end
    Transport-->>App: ChannelMessage

    Note over Peer,Transport: Inbound path (chained via draining promise)
    Peer->>Transport: onMessage(payload)
    Transport->>Transport: draining.then(ingest(payload))
    Transport->>Transport: decrypt(payload.encrypted)
    Transport->>Transport: JSON.parse → ChannelMessage
    alt channelId mismatch
        Transport->>Transport: ignore
    else seq le appliedSeq
        Transport->>Transport: drop (duplicate)
    else "seq = appliedSeq + 1"
        Transport->>Transport: buffer.set(seq, msg) → drain()
        Transport->>Session: receiveIncoming(msg)
        Transport->>Transport: "appliedSeq = seq"
        Transport->>App: onMessage(msg)
    else seq gt appliedSeq + 1 (gap)
        Transport->>Transport: buffer.set(seq, msg) — held until gap fills
    end
Loading

Reviews (1): Last reviewed commit: "test(l2ps): WI-A end-to-end with real id..." | Re-trigger Greptile

Comment on lines +205 to +211
const key = await crypto.subtle.importKey(
"raw",
this.sharedKey.buffer as ArrayBuffer,
"AES-GCM",
false,
["encrypt"],
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Wrong key material when sharedKey is a typed-array subview

this.sharedKey.buffer gives the entire underlying ArrayBuffer, ignoring byteOffset and byteLength. If sharedKey was created as a view into a larger buffer (e.g. a KDF output slice: new Uint8Array(kdfOutput, 32, 32)), the full backing buffer is handed to importKey instead of the 32-byte window, producing a silently different AES key. Encryption would succeed, decryption on the other party would fail with an authentication error at runtime. The safe form is to pass the Uint8Array directly — Web Crypto's BufferSource accepts typed-array views and correctly respects their offset/length.

Suggested change
const key = await crypto.subtle.importKey(
"raw",
this.sharedKey.buffer as ArrayBuffer,
"AES-GCM",
false,
["encrypt"],
)
const key = await crypto.subtle.importKey(
"raw",
this.sharedKey,
"AES-GCM",
false,
["encrypt"],
)

Comment on lines +224 to +230
const key = await crypto.subtle.importKey(
"raw",
this.sharedKey.buffer as ArrayBuffer,
"AES-GCM",
false,
["decrypt"],
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Same sharedKey.buffer subview problem in decrypt — pass the Uint8Array directly so the view's byteOffset and byteLength are respected.

Suggested change
const key = await crypto.subtle.importKey(
"raw",
this.sharedKey.buffer as ArrayBuffer,
"AES-GCM",
false,
["decrypt"],
)
const key = await crypto.subtle.importKey(
"raw",
this.sharedKey,
"AES-GCM",
false,
["decrypt"],
)

Comment on lines +184 to +192
while (next) {
this.buffer.delete(next.sequence)
// Throws on tamper (bad signature, wrong sender, channelId
// mismatch) — propagate as channel-fatal per §8.12.
await this.session.receiveIncoming(next)
this.appliedSeq = next.sequence
this.onMessage?.(next)
next = this.buffer.get(this.appliedSeq + 1)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Buffered messages silently stranded after a verification failure

drain() calls this.buffer.delete(next.sequence) before awaiting receiveIncoming. If receiveIncoming throws (bad signature, sender-not-in-members, etc.), the message is gone from the buffer but appliedSeq is not advanced. All subsequently buffered messages (sequence N+1, N+2, …) expect the gap at N to fill; since it never will, drain() returns immediately on every future call and those messages are silently held forever — with no further onError fired. The PR documents this as "channel-fatal per §8.12", but callers only learn of the lethality via a single error and then observe silence. Either move the buffer.delete after a successful receiveIncoming, or add a stopped flag that gate-drops all future inbound frames with a clear error.

Comment on lines +137 to +153
async send(opts: {
type: ChannelMessageType
body: unknown
repliesTo?: number
}): Promise<ChannelMessage> {
const signed = await this.session.sendOutgoing(opts)
// Our own send advances the shared per-channel counter; record it
// so we expect the peer's reply at appliedSeq + 1.
if (signed.sequence > this.appliedSeq) this.appliedSeq = signed.sequence

const encrypted = await this.encrypt(JSON.stringify(signed))
const messageHash = signed.signature.signature // unique per signed envelope
for (const to of this.recipients) {
await this.peer.send(to, encrypted, messageHash)
}
return signed
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 send() updates appliedSeq outside the serialised draining chain

send() reads and then writes appliedSeq synchronously (after the sendOutgoing await) while the inbound draining promise chain can be mid-flight concurrently. In the turn-based DACS model (offer → counter → accept) a party never sends while expecting an inbound, so this race is out-of-scope for SR-4 v1. The gap is nonetheless real: any caller that issues a send() while an inbound is mid-drain can see appliedSeq advanced by drain() before send() writes it, or vice-versa. A lightweight fix is to route send()'s appliedSeq update through a helper that also chains onto draining.

@sonarqubecloud

Copy link
Copy Markdown

@tcsenpai

Copy link
Copy Markdown
Contributor

Auto-closed by the merge of #99 (combined WI-A/B/C, merged as 2a6baa3). All Greptile findings from this slice were addressed there (5/5).

@tcsenpai tcsenpai deleted the feat/sr4-wi-a-channel-transport branch June 24, 2026 19:16
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.

2 participants