net: carry contextKey on SocketBackend's private registration (fixes #587)

[Yaraslaut]

Fixes #587.

What was wrong

morph::net::SocketBackend is a wire-protocol backend that dropped contextKey on private registration, on both of its edges:

• the native bindModel path added by net: put SocketBackend on the structural registration surface natively #586 called wire::makeRegister(request.typeId) — the key discarded by omission at the call site, makeRegister's second parameter being defaulted;
• the blocking path never overrode registerModelWithContext, so it inherited IBackend's default, which does (void)contextKey;.

The shared and attach shapes (makeRegisterShared, makeAttach) already carried it, in the same function. Only the private shape was affected.

Why it matters more than the title says

RemoteServer::attachLogIfConfigured (include/morph/core/remote.hpp:704) returns without consulting the LogProvider at all when env.contextKey is empty:

if (env.contextKey.empty()) {
    return;
}

So this was not "an audit entry missing its entity key" — no log was attached, and the instance produced no audit record at all, while the identical registration over SimulatedRemoteBackend (which does override the verb, remote.hpp:1849) produced one. Two backends documented as interchangeable disagreed, and the one that disagreed failed open.

The fix

include/morph/net/socket_backend.hpp:

• bindModel's private branch now sends makeRegister(request.typeId, request.contextKey), and the comment that used to explain the drop (and point at net: SocketBackend::bindModel drops contextKey on a private registration, unlike SimulatedRemoteBackend #587) is gone with it;
• registerModelWithContext is overridden, mirroring SimulatedRemoteBackend::registerModelWithContext. It reuses this backend's existing sendControlForId(env, "register"), whose body is byte-for-byte what the old inlined one did, so the error strings ("register failed: <msg>", "register failed: disconnected") and the one-synchronous-call-at-a-time constraint are unchanged;
• registerModel forwards to it with an empty key.

This is not a judgement call: backend.hpp:155's doc comment on the default already states the rule — "Backends whose model instances live behind a wire protocol (SimulatedRemoteBackend) override this to carry @p contextKey across."

git log -S on the call site returns only 70792bee (#586), so the drop was a knowingly-preserved pre-existing gap, not an old deliberate decision this PR is undoing. #586 was right not to fold it in.

Also fixed, by routing: registerModelShared and attachModel both degrade to registerModelWithContext when primary is empty. Those are private registrations too, and they dropped the key for the same reason; they now carry it. Nothing else about those verbs changes.

Verification — reproduced, not inferred

The issue and its triage comment were both explicit that the end-to-end path had never been reproduced: the server-side half was pinned by tests/test_action_log_phase2.cpp:347, but the transport half was read, not run. That is what the new test in tests/net/test_socket_backend.cpp does — a real SocketServer over a RemoteServer with a LogProvider installed, and a real SocketBackend client over a real TCP connection.

Measured with the test applied and the fix reverted (include/morph/net/socket_backend.hpp restored to its 992b190c state), Clang 22.1.8, Debug, -DMORPH_BUILD_NET=ON:

tests/net/test_socket_backend.cpp:2191: FAILED:
  CHECK( requestedFor == std::vector<std::string>{"SbEchoModel:acct-587"} )
with expansion:
  {  } == { "SbEchoModel:acct-587" }

tests/net/test_socket_backend.cpp:2218: FAILED:
  REQUIRE( entries.size() == 1 )
with expansion:
  0 == 1

tests/net/test_socket_backend.cpp:2227: FAILED:
  CHECK( requestedFor == std::vector<std::string>{"SbEchoModel:acct-blocking"} )
with expansion:
  {  } == { "SbEchoModel:acct-blocking" }

test cases:  1 |  0 passed | 1 failed
assertions: 15 | 12 passed | 3 failed

With the fix applied:

All tests passed (17 assertions in 1 test case)

The "would this still pass if the feature did nothing" check. Registration succeeded before the fix and succeeds after, so nothing in the new test asserts on it. Every assertion is on requestedFor (was the provider consulted, and with which key) or on the journal entry's entityKey — which is precisely what failed above. The bindModel edge and the registerModelWithContext edge are separate SECTIONs, so reverting either one alone fails its own section rather than being masked by the other. A third section pins the negative: plain registerModel still sends no key and the provider is still not consulted, so the fix did not simply start stamping a key onto everything.

Full local runs, Clang 22.1.8, Debug, MORPH_BUILD_NET=ON, MORPH_BUILD_TESTS=ON:

• morph_net_tests: All tests passed (1111 assertions in 190 test cases)
• morph_tests: test cases: 1516 | 1515 passed | 1 failed as expected, assertions: 22461 | 22460 passed | 1 failed as expected (the expected failure is the pre-existing default-sink-coverage shouldfail case)
• ctest -j4: 100% tests passed out of 1708

clang-tidy-diff.py over origin/master...HEAD with CI's flags (-extra-arg=-std=c++23 -extra-arg=-Wno-missing-include-dirs, clang-tidy 22.1.8): zero findings. That result is evidence rather than a filter artifact — a control run of the same binary with --header-filter aimed at the changed header reports 17 findings inside it (all pre-existing, on unchanged lines, which the line filter drops), so the header was genuinely analysed and my new lines genuinely produced nothing. clang-format (22.1.8, repo .clang-format) applied; tree clean under it.

Review reasoning

Copilot review is unavailable on this org (zero seats), so this section is the review.

• Every already-correct path is untouched. The shared and attach envelopes are unchanged. registerModel's observable contract is identical: sendControlForId(env, "register") produces the same two error strings from the same two branches as the body it replaces, which the existing disconnect/error cases in tests/net/test_socket_backend.cpp cover and which still pass.
• No wire or protocol change, and no server change. makeRegister's contextKey already round-trips through encode/decode (pinned at test_action_log_phase2.cpp:292), and RemoteServer's register handler already reads it.
• Session stamping intact on both edges. The new override sets env.session = currentSession() before sending, as every other control verb here does; sendControlAsync stamps it itself, so the bindModel branch was already covered.
• Nothing is gated on the key being empty. The only server-side behavioural difference a non-empty contextKey makes is the LogProvider lookup — which is the point — so a caller that never set one is bit-for-bit unaffected. The new test's third section is the standing check on that.
• No new blocking, no new lock. registerModelWithContext takes the same sendSync token registerModel already took; bindModel still never enters sendSync, so the property docs/spec/core/backend.md's deadlock argument rests on ("a native control call never parks on _syncCv") is unchanged. The existing [registration-surface] tests that pin it still pass.
• The bridge.hpp seam is not touched. This change is entirely at SocketBackend's own call sites and goes nowhere near Bridge::registerHandlerImpl, so core: #567's registration surface gives Bridge no way to tell a natively non-blocking backend from a blocking one, and two shipped backends now need opposite behaviour at that one call site #593 is neither widened nor depended on.
• Doc-comment discipline. -Wdocumentation with -Werror is on: the new override's factory parameter is unnamed, so it carries no @param factory — the same convention SimulatedRemoteBackend's override follows. The build caught a first draft that did, and it is fixed.

Spec

docs/spec/core/backend.md is updated in its own commit. Three spots stated —
correctly before this branch, incorrectly after it — that SimulatedRemoteBackend
is the only backend overriding registerModelWithContext:

1. IBackend method table. Now names both wire backends, and says why the
   override is not cosmetic: attachLogIfConfigured skips the LogProvider
   lookup entirely on an empty key, so dropping it leaves the instance with no
   action log rather than a log missing a field.
2. SocketBackend's API reference. Gains a registerModelWithContext row;
   registerModel becomes the empty-key forwarder it now is; bindModel's row
   records that every shape carries request.contextKey, the private one
   included. The empty-primary degrades of registerModelShared/attachModel
   are named where they land.
3. Design decisions. The row for the permissive default now says what that
   permissiveness costs, since it is what let this ship unnoticed.

Scope deliberately held to what this branch establishes: nothing here touches
bindModel's blocking/non-blocking question (#593), and :1215 — "registerModelWithContext
is not overridden", about QtWebSocketBackend — is left alone because it is
still accurate. That gap is real and separate, filed as #594.

Verified: scripts/check_spec_sync.sh over this branch's file list reports
Spec sync OK: 10 sub-domain(s) classified, and scripts/check_spec_citations.sh
reports Prose lint OK across 837 references and 73 cited sections.

Collision note. PR #585 also edits docs/spec/core/backend.md and is blocked
on #593. The coordinator lifted the hold on this file deliberately rather than
stall correct work behind a blocked PR, and is recording the collision on #585 so
its next rebase expects it. #585 already reconciled this same file once against
#586.

Filed, not folded

• qt: QtWebSocketBackend drops contextKey on a blocking private registration, disagreeing with its own async path #594 — QtWebSocketBackend has the same gap on its blocking private registration and disagrees with its own async path, which does carry the key. Because asyncRegistrationEnabled defaults to false, the default configuration of the Qt transport does not journal private registrations — and that flag, documented purely as a blocking/non-blocking choice, is what silently decides it. Filed as inferred from reading the code, not reproduced (I did not build the Qt transport), with the corroboration that is already in the tree named explicitly. It lives in qt: move QtWebSocketBackend onto the structural registration surface #585's files, so it needs sequencing after that PR.

Also in this branch

scripts/branch_partial_allowlist.json — the default: coverage disposition for
socket_backend.hpp pins a line number as a hint, and the header edit moved that
label from 813 to 826. The gate's own message says the disposition itself is
sound ("The text still matches, so nothing is wrong with the disposition —
update the line hint") and the pinned source text is unchanged, so this is a
hint refresh, not a new or widened suppression. No entry is added, removed or
reworded; sed -n '826p' on the header prints default:.

Conflicts

• CHANGELOG.md with the render/forms: match the negative sign as locale data, not as the byte '-' (fixes #583) #598 lane's entry under the same ### Fixed heading —
  resolve additively, keeping both.
• docs/spec/core/backend.md with qt: move QtWebSocketBackend onto the structural registration surface #585, as described above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!