feat(signaling): accept the portal's flat dialect on decode (C4) — ⚠ build in flight #54

Merged

triform-admin merged 2 commits from c4/wire-envelope-dialects into main 2026-07-30 21:21:47 +00:00

Conversation 2 Commits 2 Files changed 7 +348 -39

triform-admin commented 2026-07-30 16:31:52 +00:00

Owner

Copy link

C4 / Track 3. ⚠ Do not merge until the t7 build is green — capture/ cannot be compiled here, and a build is running against this branch now (currently ~54/638 targets).

The plan was wrong about two things

1. Physics is not a second dialect. I'd assumed it carried serde aliases to strip. It doesn't — SignalingEnvelope is #[serde(tag="type", rename_all="snake_case")] over variants Offer/Answer/Ice/…, emitting exactly "offer", "answer", "ice": byte-identical to this codec's accept-list. The sdp_offer strings in that file are log labels (lines 1072, 1096), not wire tags. The plan's "delete physics's serde aliases" step has nothing to delete.

2. The portal is the real second dialect, and genuinely incompatible (browser_screencast_webrtc.rs:48-53):

{"type":"sdp_offer",     "sdp":"v=0..."}
{"type":"ice_candidate", "candidate":"...", "sdpMid":"0", "sdpMLineIndex":0}

Flat: no from, payload inlined. Decode() rejected it on all three counts. It works today only because physics translates in the middle.

What this does — and explicitly does not

Teaches Decode() the flat dialect, normalizing to the same Envelope. Encode is untouched: we always emit canonical, because an emitter choosing a dialect per-peer would need to know which peer it's addressing, which this codec deliberately does not.

It does not make pattern_c_envelope_to_portal_msg deletable, contrary to the plan's ordering. That translator is lossy the other way — it returns None for bye, request_renegotiate, probe_result, session_unhealthy, so those four never reach the portal at all. Removing it needs the portal to learn them first. Two changes, not one; conflating them would silently drop frames.

Design points worth review

• PortalTagFromString is separate from TagFromString, and a test asserts the tag spaces stay disjoint. Decode() consults both — that's where the widening lives. Keeping it out of TagFromString is what stops "canonical" quietly becoming "anything we accept".
• The accept-list stays closed, three tags wider. New negative test uses restart_ice — well-formed in every respect except its tag.
• from is inferred as kClient for flat frames. That's a statement (the portal is the only producer), not a default — callers switch on from, so leaving it indeterminate wasn't an option.
• A flat ice_candidate with no candidate is end-of-candidates; canonical says that with data: null, which has no flat equivalent.

About the test I deleted

RejectsHistoricSdpOfferTag called sdp_offer "the historic v0 tag" and existed so that flipping this contract required an explicit argument rather than a quiet edit. The framing was wrong on the facts — it's neither historic nor v0, it's live — and this PR is that argument, made where the tripwire pointed.

Lockstep changes

• conformance/suites/signaling-wire.mjs: sdp_offer moves from REJECTED_TAGS to a new PORTAL_TAGS. Without this the kit would assert the opposite of the code it ships beside.
• docs/protocols/signaling-envelope.md: new — there was no envelope spec at all, which was defensible with one dialect and an audience holding the tree. It isn't now.

Verification status

• ✅ make verify, cxx-include-lint clean (121 files), conformance 23/23
• ⏳ compile: unverified — t7 build in flight; this PR is not mergeable until it reports

C4 / Track 3. **⚠ Do not merge until the t7 build is green** — `capture/` cannot be compiled here, and a build is running against this branch now (currently ~54/638 targets). ## The plan was wrong about two things **1. Physics is not a second dialect.** I'd assumed it carried serde aliases to strip. It doesn't — `SignalingEnvelope` is `#[serde(tag="type", rename_all="snake_case")]` over variants `Offer`/`Answer`/`Ice`/…, emitting exactly `"offer"`, `"answer"`, `"ice"`: byte-identical to this codec's accept-list. The `sdp_offer` strings in that file are **log labels** (lines 1072, 1096), not wire tags. The plan's "delete physics's serde aliases" step has nothing to delete. **2. The portal is the real second dialect**, and genuinely incompatible (`browser_screencast_webrtc.rs:48-53`): ```json {"type":"sdp_offer", "sdp":"v=0..."} {"type":"ice_candidate", "candidate":"...", "sdpMid":"0", "sdpMLineIndex":0} ``` Flat: no `from`, payload inlined. `Decode()` rejected it on all three counts. It works today only because physics translates in the middle. ## What this does — and explicitly does not Teaches `Decode()` the flat dialect, normalizing to the same `Envelope`. **Encode is untouched**: we always emit canonical, because an emitter choosing a dialect per-peer would need to know which peer it's addressing, which this codec deliberately does not. It does **not** make `pattern_c_envelope_to_portal_msg` deletable, contrary to the plan's ordering. That translator is lossy the *other* way — it returns `None` for `bye`, `request_renegotiate`, `probe_result`, `session_unhealthy`, so those four never reach the portal at all. Removing it needs the portal to learn them first. Two changes, not one; conflating them would silently drop frames. ## Design points worth review - **`PortalTagFromString` is separate from `TagFromString`**, and a test asserts the tag spaces stay disjoint. `Decode()` consults both — that's where the widening lives. Keeping it out of `TagFromString` is what stops "canonical" quietly becoming "anything we accept". - **The accept-list stays closed**, three tags wider. New negative test uses `restart_ice` — well-formed in every respect except its tag. - **`from` is inferred as `kClient`** for flat frames. That's a statement (the portal is the only producer), not a default — callers switch on `from`, so leaving it indeterminate wasn't an option. - A flat `ice_candidate` with no `candidate` is end-of-candidates; canonical says that with `data: null`, which has no flat equivalent. ## About the test I deleted `RejectsHistoricSdpOfferTag` called `sdp_offer` "the historic v0 tag" and existed *so that flipping this contract required an explicit argument rather than a quiet edit*. The framing was wrong on the facts — it's neither historic nor v0, it's live — and this PR is that argument, made where the tripwire pointed. ## Lockstep changes - `conformance/suites/signaling-wire.mjs`: `sdp_offer` moves from `REJECTED_TAGS` to a new `PORTAL_TAGS`. Without this the kit would assert the opposite of the code it ships beside. - `docs/protocols/signaling-envelope.md`: **new** — there was no envelope spec at all, which was defensible with one dialect and an audience holding the tree. It isn't now. ## Verification status - ✅ `make verify`, `cxx-include-lint` clean (121 files), conformance 23/23 - ⏳ **compile: unverified** — t7 build in flight; this PR is not mergeable until it reports

triform-admin added 2 commits 2026-07-30 16:31:52 +00:00

feat(signaling): accept the portal's flat dialect on decode (C4) ... 03a24f523b

⚠ UNVERIFIED C++ — capture/ cannot be compiled here (no local Chromium; a
build is 4-8h cold). A t7 build is running against this branch; do not merge
until it is green.

The research first, because the original plan was wrong about two things:

1. Physics is NOT a second dialect. webrtc_signaling.rs's SignalingEnvelope
   is #[serde(tag="type", rename_all="snake_case")] over variants Offer /
   Answer / Ice / …, so it emits exactly "offer", "answer", "ice" — byte
   identical to this codec's accept-list. The `sdp_offer` strings in that
   file are LOG LABELS (lines 1072, 1096), not wire tags. The plan's
   "delete physics's serde aliases" step has nothing to delete.

2. The PORTAL is the real second dialect, and it is genuinely incompatible.
   portal/src/canvas/browser_screencast_webrtc.rs:48-53:

       {"type":"sdp_offer",     "sdp":"v=0..."}
       {"type":"sdp_answer",    "sdp":"v=0..."}
       {"type":"ice_candidate", "candidate":"...", "sdpMid":"0", …}

   Flat: no `from`, payload inlined as siblings of `type`. Decode() rejected
   it on all three counts. It works today only because physics translates in
   the middle (pattern_c_envelope_to_portal_msg, screencast_ws.rs:6909).

This change teaches Decode() the flat dialect and normalizes it to the same
Envelope struct. Encode is untouched: we still always EMIT canonical, because
an emitter that picked a dialect per-peer would need to know which peer it is
addressing, which this codec deliberately does not.

What this does NOT do, stated because the plan implied otherwise: it does not
make pattern_c_envelope_to_portal_msg deletable. That translator is LOSSY in
the other direction — it returns None for bye, request_renegotiate,
probe_result and session_unhealthy, so those four tags never reach the portal
at all. Removing it needs the portal to learn those tags first. "Liberalize
the decoder" and "delete the translator" are two changes, not one.

Design notes:

* PortalTagFromString is a SEPARATE function from TagFromString, and the
  test asserts the two tag spaces stay disjoint. Decode() consults both;
  that is where the widening lives. Keeping it out of TagFromString is what
  stops "canonical" quietly becoming "anything we accept".

* The accept-list stays CLOSED — three tags wider, not open. A new negative
  test uses `restart_ice`, well-formed in every respect except its tag.

* `from` is absent in this dialect and is inferred as kClient. That is a
  statement (the portal is the browser-facing UI and the only producer of
  these frames), not a default — a future flat-but-not-client producer must
  send canonical instead. Callers do switch on `from`, so leaving it
  indeterminate was not an option.

* A flat `ice_candidate` with no `candidate` field is the end-of-candidates
  marker: canonical says that with `data: null`, which has no flat
  equivalent because the payload IS the frame.

The old RejectsHistoricSdpOfferTag test called sdp_offer "the historic v0
tag" and existed so that flipping the contract required an explicit argument
rather than a quiet edit. The framing was wrong on the facts — it is neither
historic nor v0, it is live — and this commit is that argument, made in the
place the tripwire pointed at.

conformance/suites/signaling-wire.mjs moves in lockstep: sdp_offer leaves
REJECTED_TAGS for a new PORTAL_TAGS export. Without that the kit would
assert the opposite of the code it ships beside.

Verified locally: make verify passes, cxx-include-lint clean (121 files),
conformance 23/23. NOT verified: that any of this compiles.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs(protocols): write down the signaling envelope, now that there are two dialects ... 59cf78290f

There was no envelope spec in docs/protocols/ — cb_wire_envelope.h was the
only description, which was defensible while there was exactly one dialect
and the audience was people with the tree open. C4 adds a second, and "read
the C++" is not reasonable advice for someone implementing a peer against
this project from outside.

Documents both forms, the closed accept-list, and the one thing a new
implementation is most likely to get wrong (accepting an unknown tag is
invisible until it meets a peer that assumes the documented behaviour).

Also records the known gap the code comments carry: physics's
pattern_c_envelope_to_portal_msg drops bye / request_renegotiate /
probe_result / session_unhealthy, so liberalizing this decoder does not on
its own make that translator deletable.

Header stays authoritative; the doc says so explicitly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

triform-admin commented 2026-07-30 16:42:12 +00:00

Author

Owner

Copy link

✅ Build green — the C++ is no longer unverified

t7 compile of this branch:

=== STEP 6/10 autoninja OK (654s) ===
10m52.90s Build Succeeded: 618 steps - 0.95/s
=== STEP 7/10 unit tests OK (1s) ===
=== STEP 10/10 cdp validation OK (0s) ===

All ten steps passed. The portal-dialect decoder compiles against refs/branch-heads/7727.

Two things the build surfaced that reviewers should know

1. cb_wire_envelope_test.cc is NOT in any test target. STEP 7 runs exactly two binaries:

+ /work/.../cloud_browser_encoder_unittests
+ /work/.../cloud_browser_framesink_capturer_unittests

So the tests I added — and the ones that were already there — compiled but did not execute. My earlier claim that this codec's tests "lock" the contract was true of the source and false of CI. The negative test that supposedly guarded sdp_offer has never actually run in the build lane.

That is a separate gap from this PR and I'd rather flag it than quietly fold it in: adding a cloud_browser_signaling_unittests target changes what the build lane does, and deserves its own change and its own review. Filed as follow-up work.

2. Two [chromium-style] warnings on cb_wire_envelope.h (lines 164, 217 — "Complex class/struct needs an explicit out-of-line constructor/destructor"). I checked whether they were mine: they point at IceCandidatePayload and Envelope, both pre-existing and untouched by this patch. The line numbers only moved because my comments shifted the file. Not introduced here, not fixed here.

Verification summary

compiles	✅ 618 steps, all 10 build stages
make verify	✅
cxx-include-lint	✅ clean, 121 files
conformance self-tests	✅ 23/23
new unit tests executed	❌ no — the target isn't wired into the build

The last row is why I'm not calling this fully proven. The decoder compiles and the logic is straightforward, but the assertions I wrote have not run anywhere.

## ✅ Build green — the C++ is no longer unverified t7 compile of this branch: ``` === STEP 6/10 autoninja OK (654s) === 10m52.90s Build Succeeded: 618 steps - 0.95/s === STEP 7/10 unit tests OK (1s) === === STEP 10/10 cdp validation OK (0s) === ``` All ten steps passed. The portal-dialect decoder compiles against `refs/branch-heads/7727`. ## Two things the build surfaced that reviewers should know **1. `cb_wire_envelope_test.cc` is NOT in any test target.** STEP 7 runs exactly two binaries: ``` + /work/.../cloud_browser_encoder_unittests + /work/.../cloud_browser_framesink_capturer_unittests ``` So the tests I added — and the ones that were already there — **compiled but did not execute**. My earlier claim that this codec's tests "lock" the contract was true of the source and false of CI. The negative test that supposedly guarded `sdp_offer` has never actually run in the build lane. That is a separate gap from this PR and I'd rather flag it than quietly fold it in: adding a `cloud_browser_signaling_unittests` target changes what the build lane does, and deserves its own change and its own review. Filed as follow-up work. **2. Two `[chromium-style]` warnings on `cb_wire_envelope.h`** (lines 164, 217 — "Complex class/struct needs an explicit out-of-line constructor/destructor"). I checked whether they were mine: they point at `IceCandidatePayload` and `Envelope`, both **pre-existing** and untouched by this patch. The line numbers only moved because my comments shifted the file. Not introduced here, not fixed here. ## Verification summary | | | |---|---| | compiles | ✅ 618 steps, all 10 build stages | | `make verify` | ✅ | | `cxx-include-lint` | ✅ clean, 121 files | | conformance self-tests | ✅ 23/23 | | **new unit tests executed** | ❌ **no** — the target isn't wired into the build | The last row is why I'm not calling this fully proven. The decoder compiles and the logic is straightforward, but the assertions I wrote have not run anywhere.

triform-admin referenced this pull request 2026-07-30 17:03:21 +00:00

fix(build): run every unittest target that was built, not two hardcoded ones #56

triform-admin commented 2026-07-30 18:38:11 +00:00

Author

Owner

Copy link

✅ Verified: the portal-dialect assertions compile, run, and pass

This took three build iterations because each fix uncovered the next defect. Final run, with the build machinery from #56 + #57 underneath:

+ /work/.../cloud_browser_encoder_unittests          SUCCESS: all tests passed.
+ /work/.../cloud_browser_framesink_capturer_unittests  SUCCESS: all tests passed.
+ /work/.../cb_wire_envelope_unittests               SUCCESS: all tests passed.
=== STEP 7/10 unit tests OK ===

32/32 wire-envelope tests pass, including all five new ones:

CbWireEnvelopeDecodeTest.AcceptsPortalFlatOffer
CbWireEnvelopeDecodeTest.AcceptsPortalFlatIce
CbWireEnvelopeDecodeTest.PortalFlatIceWithoutCandidateIsEndOfCandidates
CbWireEnvelopeDecodeTest.RejectsUnknownTagEvenWithValidShape
CbWireEnvelopeEncodeTest.AlwaysEmitsCanonicalNeverPortal

(32, not main's 28: five added, one — RejectsHistoricSdpOfferTag — replaced.)

A false positive I nearly reported

The run before this one also said "SUCCESS, cb_wire_envelope_unittests invoked, all tests passed" — and I nearly posted it here. One name in the output stopped me: RejectsHistoricSdpOfferTag, the test this PR deletes. And no AcceptsPortalFlat* anywhere.

That branch carried the two build fixes but not C4 itself, so it had verified main's codec. grep -c PortalTagFromString on it returns 0. The claim would have been true-but-hollow: the assertions that actually needed proving never ran.

Worth stating because "the tests passed" is exactly the kind of green that has been wrong three times in this chain already.

Dependency

Do not merge before #57 and #56. Without #57 the test does not compile (//base was a private dep); without #56 STEP 7 never invokes it. On main alone, this PR's tests would build-and-vanish — the original problem.

Verification table

compiles	✅ 620 steps
tests invoked	✅ named in STEP 7
tests pass	✅ 32/32, portal assertions among them
make verify / include-lint / conformance	✅

## ✅ Verified: the portal-dialect assertions compile, run, and pass This took three build iterations because each fix uncovered the next defect. Final run, with the build machinery from #56 + #57 underneath: ``` + /work/.../cloud_browser_encoder_unittests SUCCESS: all tests passed. + /work/.../cloud_browser_framesink_capturer_unittests SUCCESS: all tests passed. + /work/.../cb_wire_envelope_unittests SUCCESS: all tests passed. === STEP 7/10 unit tests OK === ``` **32/32** wire-envelope tests pass, including all five new ones: ``` CbWireEnvelopeDecodeTest.AcceptsPortalFlatOffer CbWireEnvelopeDecodeTest.AcceptsPortalFlatIce CbWireEnvelopeDecodeTest.PortalFlatIceWithoutCandidateIsEndOfCandidates CbWireEnvelopeDecodeTest.RejectsUnknownTagEvenWithValidShape CbWireEnvelopeEncodeTest.AlwaysEmitsCanonicalNeverPortal ``` (32, not main's 28: five added, one — `RejectsHistoricSdpOfferTag` — replaced.) ## A false positive I nearly reported The run before this one also said "SUCCESS, cb_wire_envelope_unittests invoked, all tests passed" — and I nearly posted it here. One name in the output stopped me: **`RejectsHistoricSdpOfferTag`**, the test this PR *deletes*. And no `AcceptsPortalFlat*` anywhere. That branch carried the two build fixes but **not C4 itself**, so it had verified main's codec. `grep -c PortalTagFromString` on it returns 0. The claim would have been true-but-hollow: the assertions that actually needed proving never ran. Worth stating because "the tests passed" is exactly the kind of green that has been wrong three times in this chain already. ## Dependency **Do not merge before #57 and #56.** Without #57 the test does not compile (`//base` was a private dep); without #56 STEP 7 never invokes it. On main alone, this PR's tests would build-and-vanish — the original problem. ## Verification table | | | |---|---| | compiles | ✅ 620 steps | | tests invoked | ✅ named in STEP 7 | | tests pass | ✅ 32/32, portal assertions among them | | `make verify` / include-lint / conformance | ✅ |

triform-admin referenced this pull request 2026-07-30 18:38:32 +00:00

fix(build): run every unittest target that was built, not two hardcoded ones #56

triform-admin referenced this pull request 2026-07-30 19:29:41 +00:00

feat(cdp): reject unknown Cb.startNativeSession params instead of ignoring them (C5) #58

triform-admin merged commit 97bd72498e into main 2026-07-30 21:21:47 +00:00

triform-admin referenced this pull request from a commit 2026-07-30 21:21:47 +00:00

Merge pull request 'feat(signaling): accept the portal's flat dialect on decode (C4) — ⚠ build in flight' (#54) from c4/wire-envelope-dialects into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

triform/chromeless!54

No description provided.