connect: expose light mode and confirmed reads #33

Merged

yuris merged 2 commits from feat/22-light-confirmed into main 2026-08-19 16:22:17 +00:00

Conversation 1 Commits 2 Files changed 5 +261 -11

yuris commented 2026-08-19 15:54:29 +00:00

Owner

Copy link

Closes #22. Review actioned; body rewritten after review — several original claims were superseded.

Two connect options that v1.json.spacetimedb supports and this SDK could not ask for:

• light (default false) → ?light=true. The server sends TransactionUpdateLight for transactions this client did not itself call. The caller always still receives a full update — now proven against a live server, not read from source.
• confirmed_reads (default nil) → ?confirmed=true|false. Tri-state: absent, true and false are three distinct requests. Justified as forward compatibility — V1 resolves absent and confirmed=false identically, but later protocol versions default absent to true, so a boolean option defaulting to false would have frozen V1's meaning into the API.

Both options reach the URL on both platforms — native (header auth) and HTML5 (token in URL).

The refactor

with_token assumed it was the only query parameter. There is now one private with_param(url, key, value) making the ? vs & decision, with with_token a single line over it — same signature, byte-identical output on every non-erroring input (verified across 10 inputs including an existing query string, an empty token, and a URL ending in ?).

What review changed

• subscribe_url is private, not public API. Its third argument is not opts.token but the platform decision connect already made, so a public version invites stdb.subscribe_url(opts.url, opts) → token silently dropped → on HTML5, a successful connection under a brand-new anonymous identity. That is the #8/#17 failure mode exactly. The reviewer's own reductio settled it: dropping the third argument makes it unusable by connect, which proves it is internal.
• URL tests now drive connect with stubbed _G.websocket / _G.timer / is_html5, instead of testing a public seam. This closed a real coverage hole: moving the URL composition back inside the if token block would have silently broken native light mode with a fully green suite. Verified by doing exactly that — one test goes red.
• Boolean validation. opts.confirmed_reads and "true" or "false" collapsed every truthy value, so confirmed_reads = "false" — a string, as it arrives from a config file or json.decode — requested confirmed=true. An exact inversion. Now asserts boolean-or-nil, with tests pinning the rejection.
• TransactionUpdateLight had zero coverage. Before this PR a v1 client could not reach that handler; this PR is what makes it reachable. Now unit-tested.
• The light-mode claim is proven on the wire. smoke.script connects with light = true, and phase 1 asserts the caller's own update arrives as a full TransactionUpdate carrying reducer_call and that the reducer callback already fired. This matters concretely: the SDK correlates callbacks on reducer_call.request_id, and handlers.TransactionUpdateLight never touches c.pending — so if a caller's own update ever arrived light, every call_reducer callback would silently never fire and c.pending would leak for the life of the connection. A baseline conformance run was taken before the change, so the assertion is not shipping unexercised.

One review claim was corrected rather than accepted: TransactionUpdateLight does carry request_id on the wire (protocol.lua:40 lists it REQUIRED). The real defect is one level over — the SDK correlates on reducer_call.request_id, which the light variant has no equivalent of. The consequence is unchanged; the docs are written against the accurate mechanism.

Verification

• 5 passed, 0 failed; stylua --check clean
• Native conformance PASS on this branch, run independently of the author: PASS (identity 0xc200d7a1…dfb0 persisted across a process restart)
• Falsifiability verified twice: (1) reverting the URL composition into the if token block goes red with exactly the predicted native light-mode failure; (2) sending light=notabool makes the live server refuse every connection, proving the parameter is genuinely parsed server-side rather than ignored — so a green run means something
• chat_schema.lua regenerated byte-identical during both conformance runs: no schema drift against the pinned server

Still deferred, deliberately

Proving the server actually sends TransactionUpdateLight for transactions this client did not call. That needs a second connection in the harness — genuinely new infrastructure. #22 explicitly offered this deferral; the single-client half is now covered, and the README names what remains unproven rather than implying full coverage.

confirmed_reads is not added to the conformance run: the README makes no server-behaviour claim about it, and setting it changes durability timing in a latency-sensitive harness. Its URL composition is unit-tested on both platforms, which is all #22 asked for.

Closes #22. **Review actioned; body rewritten after review — several original claims were superseded.** Two connect options that `v1.json.spacetimedb` supports and this SDK could not ask for: - `light` (default `false`) → `?light=true`. The server sends `TransactionUpdateLight` for transactions this client did not itself call. **The caller always still receives a full update** — now proven against a live server, not read from source. - `confirmed_reads` (default `nil`) → `?confirmed=true|false`. Tri-state: absent, true and false are three distinct requests. Justified as *forward compatibility* — V1 resolves absent and `confirmed=false` identically, but later protocol versions default absent to `true`, so a boolean option defaulting to false would have frozen V1's meaning into the API. Both options reach the URL on **both** platforms — native (header auth) and HTML5 (token in URL). ## The refactor `with_token` assumed it was the only query parameter. There is now one private `with_param(url, key, value)` making the `?` vs `&` decision, with `with_token` a single line over it — same signature, byte-identical output on every non-erroring input (verified across 10 inputs including an existing query string, an empty token, and a URL ending in `?`). ## What review changed - **`subscribe_url` is private**, not public API. Its third argument is not `opts.token` but the platform decision `connect` already made, so a public version invites `stdb.subscribe_url(opts.url, opts)` → token silently dropped → on HTML5, a *successful* connection under a brand-new anonymous identity. That is the #8/#17 failure mode exactly. The reviewer's own reductio settled it: dropping the third argument makes it unusable by `connect`, which proves it is internal. - **URL tests now drive `connect`** with stubbed `_G.websocket` / `_G.timer` / `is_html5`, instead of testing a public seam. This closed a real coverage hole: moving the URL composition back inside the `if token` block would have silently broken native light mode with a fully green suite. Verified by doing exactly that — one test goes red. - **Boolean validation.** `opts.confirmed_reads and "true" or "false"` collapsed every truthy value, so `confirmed_reads = "false"` — a string, as it arrives from a config file or `json.decode` — requested `confirmed=true`. An exact inversion. Now asserts boolean-or-nil, with tests pinning the rejection. - **`TransactionUpdateLight` had zero coverage.** Before this PR a v1 client could not reach that handler; this PR is what makes it reachable. Now unit-tested. - **The light-mode claim is proven on the wire.** `smoke.script` connects with `light = true`, and phase 1 asserts the caller's own update arrives as a full `TransactionUpdate` carrying `reducer_call` *and* that the reducer callback already fired. This matters concretely: the SDK correlates callbacks on `reducer_call.request_id`, and `handlers.TransactionUpdateLight` never touches `c.pending` — so if a caller's own update ever arrived light, every `call_reducer` callback would silently never fire and `c.pending` would leak for the life of the connection. A baseline conformance run was taken *before* the change, so the assertion is not shipping unexercised. One review claim was corrected rather than accepted: `TransactionUpdateLight` *does* carry `request_id` on the wire (`protocol.lua:40` lists it REQUIRED). The real defect is one level over — the SDK correlates on `reducer_call.request_id`, which the light variant has no equivalent of. The consequence is unchanged; the docs are written against the accurate mechanism. ## Verification - `5 passed, 0 failed`; `stylua --check` clean - Native conformance **PASS** on this branch, run independently of the author: `PASS (identity 0xc200d7a1…dfb0 persisted across a process restart)` - Falsifiability verified twice: (1) reverting the URL composition into the `if token` block goes red with exactly the predicted native light-mode failure; (2) sending `light=notabool` makes the live server refuse every connection, proving the parameter is genuinely parsed server-side rather than ignored — so a green run means something - `chat_schema.lua` regenerated byte-identical during both conformance runs: no schema drift against the pinned server ## Still deferred, deliberately Proving the server actually sends `TransactionUpdateLight` for transactions this client did *not* call. That needs a second connection in the harness — genuinely new infrastructure. #22 explicitly offered this deferral; the single-client half is now covered, and the README names what remains unproven rather than implying full coverage. `confirmed_reads` is not added to the conformance run: the README makes no server-behaviour claim about it, and setting it changes durability timing in a latency-sensitive harness. Its URL composition is unit-tested on both platforms, which is all #22 asked for.

yuris added 1 commit 2026-08-19 15:54:29 +00:00

Expose light mode and confirmed reads as connect options ... 8a4b70571d

Both are single query parameters the v1 JSON protocol already accepts, and
`connection.lua` has handled `TransactionUpdateLight` since it was written —
the SDK simply had no way to ask for either. Light mode is worth the option
on this target specifically: it strips caller identity, reducer call, energy
and timing from every transaction the client did not cause, and that is JSON
a pure-Lua decoder pays for on the main thread on mobile.

`confirmed_reads` stays tri-state rather than defaulting to `false`. Absent,
`true` and `false` are three distinct requests to the server, so a boolean
default would silently start sending `confirmed=false` on URLs where every
release so far sent nothing — a behaviour change smuggled in as a default,
and #24 freezes this API.

The actual work was query-parameter composition. `with_token` knew how to
append one parameter; with light and confirmed also in play a URL needs
`?a=1&b=2&c=3`, and on HTML5 the token is one of the three. That `?`/`&`
decision now lives in exactly one place (`with_param`), with `with_token`
routed through it and unchanged in signature and output. `subscribe_url`
composes in a fixed order (light, confirmed, token) so the result is
assertable byte-for-byte, and it is pure, which is why `connect` — which
needs a real engine — does not have to be to get these tested.

Deliberately not done: the conformance assertion that the server honours
light mode. Proving it requires a *second* connection to cause a transaction
this client did not call, since the caller always receives a full update, and
the harness runs one client. Shipping the option with unit coverage only.

yuris commented 2026-08-19 16:07:56 +00:00

Author

Owner

Copy link

Review — automated reviewer, posted for the record

Review — PR #33 feat/22-light-confirmed

Ran everything: 5 passed, 0 failed and stylua --check stdb dev/test exit 0 both reproduce. Suite runs under Lua 5.1.5; the diff uses only .., ~=, and/or and string.find(…, 1, true) — no 5.2+ constructs. The diff touches stdb/init.lua (the one shipped file allowed engine globals), README.md and dev/test/ only, so the pure-core no-Defold-globals rule is untouched.

Headline: the bug you were most worried about is not there — I verified it by running connect — but the PR ships one new frozen public function that should not be frozen, and its two most load-bearing claims have no test that could detect them breaking.

---

1. M.subscribe_url as public API — blocker (for a PR on the #24 freeze path)

Make it local. Definitively. Three independent reasons, the third decisive:

It is public in code and absent from the documented surface. README.md:337-345 lists every public facade function — including both existing testing seams, with_token and is_html5. The PR does not add subscribe_url to that table. #24's contract is "the documented surface is stable", so this lands in the worst quadrant: reachable by consumers, not covered by the freeze promise.

It duplicates with_token. subscribe_url(u, {}, t) == with_token(u, t) — the PR asserts exactly that at dev/test/test_init.lua:70-74. Freezing both means freezing two overlapping URL builders forever, one of which is a strict superset of the other.

The signature is a consumer footgun. M.subscribe_url(url, opts, token) (init.lua:65) takes a connect-opts table but ignores opts.token, while connect-opts has a token field (README.md:355). A consumer holding their opts table writes the obvious thing:

local url = stdb.subscribe_url(opts.url, opts)   -- token silently dropped

On HTML5 that yields a connection that succeeds as a brand new anonymous identity — the precise silent-wrong-answer failure of #8 and #17, which init.lua:149-153 has a five-line comment warning about. And the reason the third argument exists at all is that the platform decision lives in connect (init.lua:147-160): the token must not enter the URL on native. A function whose third parameter only makes sense to its single in-tree caller is an internal seam, not an API.

The with_token/is_html5 precedent looks like it cuts the other way, but it does not: both are self-contained concepts and documented. subscribe_url is neither.

The fix, and why it is strictly better than the current tests

Make it local function subscribe_url(url, opts, token) beside with_param; keep M.with_token public and documented as-is. Then test through connect with a stubbed websocket global. I confirmed this works in this runner — runner.lua:50 sandboxes each test file, but _G.websocket = {…} writes the real global that init.lua:113 reads, and is_html5 is already documented at init.lua:79 as "wrapped as a function so tests can stub it" (a seam nothing currently uses). Roughly 15 lines:

local captured
_G.websocket = {
  EVENT_MESSAGE = "m", EVENT_DISCONNECTED = "d", EVENT_ERROR = "e", DATA_TYPE_TEXT = "t",
  connect = function(url, params) captured = { url = url, headers = params.headers } return {} end,
  send = function() end, disconnect = function() end,
}
_G.timer = { delay = function() end }
local real_is_html5 = stdb.is_html5
local function url_for(html5, opts)
  stdb.is_html5 = function() return html5 end
  opts.url, opts.json, opts.storage = base, json, false
  stdb.connect(opts)
  return captured.url, captured.headers
end
-- restore at end of file: stdb.is_html5 = real_is_html5; _G.websocket, _G.timer = nil, nil

Restoring matters: package.loaded persists across test files in this runner.

That test removes the new public API and closes the finding-6 gap below, which no subscribe_url unit test can reach. Two problems, one smaller diff.

If you keep it public anyway: it must be added to the README.md:337-345 table, and the opts.token footgun must go — but dropping the third argument makes it unusable by connect, which is the proof it belongs private.

---

2. with_token backwards compatibility — verified byte-identical

I reimplemented the pre-PR body and diffed both against ten inputs:

input	old	new	same
no query string	…?token=tok	…?token=tok	✅
existing query (?compression=None)	…&token=tok	…&token=tok	✅
empty token	?token=	?token=	✅
JWT-ish a.b-c_d	?token=a.b-c_d	identical	✅
needs escaping a b&c=d?e#f/g+h%i	passed through raw	identical	✅
URL ending in ?	?&token=tok	?&token=tok	✅
URL with #frag	#frag?token=tok	identical	✅
number 42	?token=42	?token=42	✅
nil token	error	error	text differs
boolean token	error	error	text differs

The PR's "same signature, same output" holds for every non-erroring input. The one difference is the error string, because the concat moved into with_param and the local is named value:

old: p1.lua:3: attempt to concatenate local 'token' (a nil value)
new: stdb/init.lua:45: attempt to concatenate local 'value' (a nil value)

Nit, no action. Both raise; nothing pcalls this; the message was never a contract. Worth knowing only because the PR says byte-identical and this is the single place it is not.

Uncovered by tests, unchanged by the PR: a URL already ending in ? produces ?&token= (empty first pair). Pre-existing in both implementations; not this PR's problem.

---

3. Tri-state confirmed_reads — correct conditional, but the value mapping has the bug one level down (should-fix)

The conditional you were worried about is right. init.lua:69 is if opts.confirmed_reads ~= nil then, not if opts.confirmed_reads then. Measured:

confirmed_reads=nil     -> ws://h/sub
confirmed_reads=false   -> ws://h/sub?confirmed=false
confirmed_reads=true    -> ws://h/sub?confirmed=true

false is sent, not swallowed. Upstream confirms the tri-state is real: SubscribeQueryParams.confirmed is Option<bool> (crates/client-api/src/routes/subscribe.rs:100).

But init.lua:70 maps the value with opts.confirmed_reads and "true" or "false", so every non-false truthy value collapses to "true". Measured:

confirmed_reads=0        -> ?confirmed=true
confirmed_reads=""       -> ?confirmed=true
confirmed_reads="false"  -> ?confirmed=true      <-- exact inversion
light="false"            -> ?light=true          <-- same, init.lua:66

A value arriving from a config file, a json.decode, or a Defold .ini is a string. confirmed_reads = "false" requesting confirmed=true is the same silent-inversion class the PR body congratulates itself on avoiding for nil, just moved from the conditional into the value. On a frozen public API this is an input-validation gap at a trust boundary.

Fix, one line each:

assert(opts.light == nil or type(opts.light) == "boolean", "stdb: light must be a boolean")
assert(opts.confirmed_reads == nil or type(opts.confirmed_reads) == "boolean",
  "stdb: confirmed_reads must be a boolean or nil")

Test-honesty note on the nil case

test_init.lua:52-56 asserts subscribe_url(base, { confirmed_reads = nil }). Lua's table constructor drops nil fields — I counted the fields, it is 0 — so that table is {} and the assertion is byte-identical to test_init.lua:34. It documents intent, which is fine, but the PR body's "each confirmed_reads state including nil producing no parameter at all" over-claims it as distinct coverage. The real nil coverage is line 34.

Similarly test_init.lua:70-74 (subscribe_url(base, {}, "tok") == with_token(base, "tok")) is self-referential: both sides route through with_param, so a change to the separator logic moves both and the test still passes. What actually pins today's bytes is the pair of literal-string assertions at test_init.lua:17-27, unmodified by this PR — so the PR's claim is true, just via a different test than it credits. Cheap improvement: assert against the literal base .. "?token=tok".

---

4. Parameter escaping — absence is correct, not latent (nit: write the reasoning down)

No encoding is right here, and I checked the server side rather than assuming:

• light/confirmed values are the hardcoded literals "true"/"false". Nothing to encode.
• Upstream parses with axum's Query<SubscribeQueryParams> (subscribe.rs:120-127), i.e. serde_urlencoded, so values are percent-decoded server-side.
• A SpacetimeDB token is a JWT: base64url (A-Za-z0-9-_) plus . separators. None of those are percent-significant, and base64url never emits + or /. Decoding is a no-op for exactly the inputs this parameter carries. Your instinct is right.

Latent only for a non-JWT token containing &, =, %, + or a space — which would corrupt the query string. That is pre-existing in with_token, not a regression, and encoding it now would change with_token's output for precisely the inputs that are already broken.

Recommendation: do not encode. Add one comment line to with_param (init.lua:44) stating that values must already be URL-safe — literals and JWT base64url — so the next person adding a user-supplied parameter through this now-shared helper does not inherit a silent bug. The helper's own docstring currently advertises that everything goes through it, which makes the unstated precondition more dangerous than it was when only the token used it.

---

5. Ordering claim — accurate and genuinely pinned

init.lua:66-74 is three sequential if statements, straight-line, no table iteration — had it been a pairs loop over a params table, order would have been non-deterministic and unassertable. It is not.

Pinned by two tests that would fail on a swap: test_init.lua:58-62 (?light=true&confirmed=false) and :64-68 (?light=true&confirmed=true&token=tok). Claim verified.

---

6. Both platform paths — no bug; verified by running connect (but zero test coverage)

This is the one you flagged as most likely broken. It is correct, and the reason is init.lua:161: local url = M.subscribe_url(opts.url, opts, url_token) sits outside the if token block, so the URL is composed unconditionally. url_token stays nil on native, so the token stays out of the URL and params.headers is still set at :158.

I stubbed _G.websocket and captured what websocket.connect actually receives:

scenario	url	headers
native, light+confirmed, no token	…subscribe?light=true&confirmed=false	nil
native, light+confirmed, token	…subscribe?light=true&confirmed=false	Authorization: Bearer JWT.a-b_c
html5, light+confirmed, token	…subscribe?light=true&confirmed=false&token=JWT.a-b_c	nil
native, defaults, token	…subscribe	Authorization: Bearer JWT.a-b_c
html5, defaults, token	…subscribe?token=JWT.a-b_c	nil
native, defaults, no token	…subscribe	nil

Both transports get light/confirmed; native still authenticates by header with no token in the URL; HTML5 appends the token last; and the default rows are byte-identical to pre-PR output. Also worth noting the URL is recomputed inside open() rather than hoisted, so a reconnect does not accumulate duplicate parameters — the other classic shape of this bug is absent too.

The gap is coverage, not correctness. If someone moved :161 back inside the if token block, every test in this PR would still pass, and native builds would silently lose light mode — on the platform the README's bandwidth argument is aimed at. The pure-seam tests structurally cannot see this, because the platform branch lives in connect. This is what turns finding 1 from a taste argument into a concrete one: the stubbed-connect test both deletes the new public API and covers this.

---

7. The README claim — accurate, verified against upstream, and untested where it matters most (should-fix)

I checked the source rather than trusting the PR body. crates/core/src/subscription/module_subscription_manager.rs:

Regardless, the update that we send to the caller, if we send any, is a full tx update, rather than a light one. That is, in the case of the caller, we don't respect the light setting.

The caller branch sends TransactionUpdateMessage { event: Some(event.clone()), … }; the light gate client.config.tx_update_full.then(|| event.clone()) appears only in the non-caller loop. So "Your own reducer calls still come back as full updates" is correct, as is ?light=true → tx_update_full: !light (subscribe.rs:206).

Why that sentence is load-bearing rather than cosmetic: handlers.TransactionUpdateLight (stdb/connection.lua:324-327) applies the cache and emits transaction, but a light message carries no request_id, so unlike handlers.TransactionUpdate (:303-311) it never clears c.pending[request_id]. If a caller's own update ever arrived light, every db:call_reducer callback would silently never fire and c.pending would leak entries for the life of the connection. The README sentence is the only thing standing between this PR and that failure — and #24 says explicitly: "every claim about the wire is proven by a captured fixture, not by reading server source. This repo has been burned three times by the wire disagreeing with the docs."

The deferral is broader than #22 authorised. #22 offers deferring "did the server honour light mode", which needs a second connection to cause a transaction this client did not call. The bolded README claim needs one client: connect with light = true, call a reducer yourself, assert the update arrives as full TransactionUpdate with reducer_call present and the reducer callback fires. That fits the existing single-client harness with no new infrastructure. The PR's "Deliberately not done" section is admirably explicit about the half it names, and silently sweeps this half in with it.

Cheaper second gap, independent of any server. Nothing in dev/ mentions TransactionUpdateLight — zero grep hits across dev/, zero in dev/test/fixtures/session.jsonl. Before this PR that handler was unreachable for a v1 client; this PR is what makes it reachable. The PR body's "connection.lua already handles that message, so this adds no new message handling — only the ability to ask" is technically true and rhetorically misleading: it adds the ability to reach an entirely untested handler. Eight lines in test_connection.lua using the existing new_conn() + conn:handle_frame(json.encode{ TransactionUpdateLight = { update = … } }) pattern, asserting cache counts and the emitted transaction event, closes it.

Two smaller README accuracy points

README.md:365-367 — "nil and false are different requests to the server". At the wire level yes; behaviourally on the protocol this SDK speaks, no. resolve_confirmed_reads_default (subscribe.rs:103-111) maps WsVersion::V1 + None → false, identical to Some(false); upstream's own test pins it (subscribe.rs:2838-2844). The tri-state is still the right design — V2/V3 map None → DEFAULT_CONFIRMED_READS, which is true — so the honest justification is forward compatibility: nil means something different on the next protocol version, and a boolean default would have baked in today's meaning. One sentence, nit.

Informational, no action. Upstream also computes hide_reducer_info_for_non_callers = matches!(module_def_version, RawModuleDefVersion::V10) and sends event: None to non-callers regardless of light. For V10 modules part of light mode's saving is already unconditional, so the README's bandwidth pitch may overstate the marginal delta on some modules. Doesn't affect the change.

---

Claim audit

PR body claim	verdict
with_token same signature, same output	✅ byte-identical on all 10 non-erroring inputs; error text differs only
proven by two pre-existing tests passing unmodified	✅ test_init.lua:17-27 untouched, literal expected strings
order fixed light → confirmed → token	✅ straight-line code, pinned by two tests
tri-state nil omits the parameter	✅ ~= nil, not truthiness
eight new assertions	✅ exactly 8
"each confirmed_reads state including nil"	⚠️ the nil assertion is byte-identical to the {} one; Lua drops nil fields
default case byte-identical to today's	✅ true, but proven by the old literal tests, not by :70-74, which compares new code to new code
"adds no new message handling"	⚠️ true, but it makes a handler with zero test coverage reachable for the first time
"caller always still receives a full update"	✅ verified in upstream source — but that is exactly the kind of claim #24 forbids resting on source reads
5 passed, 0 failed; stylua clean	✅ both reproduce

---

Does it close #22?

Substantially yes, with two acceptance gaps.

Met: light alone (:36), confirmed alone (:38-42), both (:58-62), both plus token proving ?/& composition (:64-68), default byte-identical (via :17-27). Both options genuinely reach the URL on both platforms. #22's request to "say so explicitly rather than leaving it ambiguous" about the conformance deferral is honoured with a dedicated section.

Not met:

1. The acceptance criterion is only satisfied for the pure seam, not for connect. #22 asks that the options be threaded onto the URL; nothing tests that either platform branch does so. Reverting init.lua:161 into the if token block would break native light mode with a fully green suite.
2. The deferral is over-broad. The single-client half of the light-mode claim — the caller still gets a full update — is testable in the existing harness and is now asserted in bold in consumer-facing docs with nothing behind it. handlers.TransactionUpdateLight also has no unit test at all.

---

Summary

Blocker (1): make M.subscribe_url local; test through connect with a stubbed websocket global (~15 lines, verified working), which also closes gap 6. Do not freeze this signature — it takes an opts table and ignores opts.token.

Should-fix (3): assert boolean-or-nil on light/confirmed_reads ("false" currently requests true); add the single-client conformance assertion for the bolded README claim, or soften the README; add a TransactionUpdateLight unit test to test_connection.lua.

Nits (4): note the unstated URL-safe precondition in with_param; reword the nil vs false justification as forward compatibility; strengthen test_init.lua:70-74 to a literal string; the confirmed_reads = nil assertion is documentation, not coverage — worth labelling as such.

The engineering is sound and the refactor is the right one — with_param is exactly the correct shape and the compatibility is genuinely byte-identical. What needs work is the public surface it exposes on the way to a freeze, and the distance between what the README asserts and what any test can detect.

## Review — automated reviewer, posted for the record # Review — PR #33 `feat/22-light-confirmed` Ran everything: `5 passed, 0 failed` and `stylua --check stdb dev/test` exit 0 both reproduce. Suite runs under **Lua 5.1.5**; the diff uses only `..`, `~=`, `and`/`or` and `string.find(…, 1, true)` — no 5.2+ constructs. The diff touches `stdb/init.lua` (the one shipped file allowed engine globals), `README.md` and `dev/test/` only, so the pure-core no-Defold-globals rule is untouched. Headline: **the bug you were most worried about is not there** — I verified it by running `connect` — but the PR ships one new frozen public function that should not be frozen, and its two most load-bearing claims have no test that could detect them breaking. --- ## 1. `M.subscribe_url` as public API — **blocker** (for a PR on the #24 freeze path) Make it `local`. Definitively. Three independent reasons, the third decisive: **It is public in code and absent from the documented surface.** `README.md:337-345` lists every public facade function — including both existing testing seams, `with_token` and `is_html5`. The PR does not add `subscribe_url` to that table. #24's contract is *"the documented surface is stable"*, so this lands in the worst quadrant: reachable by consumers, not covered by the freeze promise. **It duplicates `with_token`.** `subscribe_url(u, {}, t) == with_token(u, t)` — the PR asserts exactly that at `dev/test/test_init.lua:70-74`. Freezing both means freezing two overlapping URL builders forever, one of which is a strict superset of the other. **The signature is a consumer footgun.** `M.subscribe_url(url, opts, token)` (`init.lua:65`) takes a connect-opts table but ignores `opts.token`, while connect-opts *has* a `token` field (`README.md:355`). A consumer holding their opts table writes the obvious thing: ```lua local url = stdb.subscribe_url(opts.url, opts) -- token silently dropped ``` On HTML5 that yields a connection that succeeds as a **brand new anonymous identity** — the precise silent-wrong-answer failure of #8 and #17, which `init.lua:149-153` has a five-line comment warning about. And the reason the third argument exists at all is that the platform decision lives in `connect` (`init.lua:147-160`): the token *must not* enter the URL on native. A function whose third parameter only makes sense to its single in-tree caller is an internal seam, not an API. The `with_token`/`is_html5` precedent looks like it cuts the other way, but it does not: both are self-contained concepts *and* documented. `subscribe_url` is neither. ### The fix, and why it is strictly better than the current tests Make it `local function subscribe_url(url, opts, token)` beside `with_param`; keep `M.with_token` public and documented as-is. Then test through `connect` with a stubbed `websocket` global. I confirmed this works in this runner — `runner.lua:50` sandboxes each test file, but `_G.websocket = {…}` writes the real global that `init.lua:113` reads, and `is_html5` is already documented at `init.lua:79` as *"wrapped as a function so tests can stub it"* (a seam nothing currently uses). Roughly 15 lines: ```lua local captured _G.websocket = { EVENT_MESSAGE = "m", EVENT_DISCONNECTED = "d", EVENT_ERROR = "e", DATA_TYPE_TEXT = "t", connect = function(url, params) captured = { url = url, headers = params.headers } return {} end, send = function() end, disconnect = function() end, } _G.timer = { delay = function() end } local real_is_html5 = stdb.is_html5 local function url_for(html5, opts) stdb.is_html5 = function() return html5 end opts.url, opts.json, opts.storage = base, json, false stdb.connect(opts) return captured.url, captured.headers end -- restore at end of file: stdb.is_html5 = real_is_html5; _G.websocket, _G.timer = nil, nil ``` Restoring matters: `package.loaded` persists across test files in this runner. That test removes the new public API *and* closes the finding-6 gap below, which no `subscribe_url` unit test can reach. Two problems, one smaller diff. **If you keep it public anyway:** it must be added to the `README.md:337-345` table, and the `opts.token` footgun must go — but dropping the third argument makes it unusable by `connect`, which is the proof it belongs private. --- ## 2. `with_token` backwards compatibility — **verified byte-identical** I reimplemented the pre-PR body and diffed both against ten inputs: | input | old | new | same | |---|---|---|---| | no query string | `…?token=tok` | `…?token=tok` | ✅ | | existing query (`?compression=None`) | `…&token=tok` | `…&token=tok` | ✅ | | empty token | `?token=` | `?token=` | ✅ | | JWT-ish `a.b-c_d` | `?token=a.b-c_d` | identical | ✅ | | needs escaping `a b&c=d?e#f/g+h%i` | passed through raw | identical | ✅ | | URL ending in `?` | `?&token=tok` | `?&token=tok` | ✅ | | URL with `#frag` | `#frag?token=tok` | identical | ✅ | | number `42` | `?token=42` | `?token=42` | ✅ | | **nil token** | error | error | text differs | | **boolean token** | error | error | text differs | The PR's *"same signature, same output"* holds for every non-erroring input. The one difference is the error string, because the concat moved into `with_param` and the local is named `value`: ``` old: p1.lua:3: attempt to concatenate local 'token' (a nil value) new: stdb/init.lua:45: attempt to concatenate local 'value' (a nil value) ``` **Nit, no action.** Both raise; nothing `pcall`s this; the message was never a contract. Worth knowing only because the PR says byte-identical and this is the single place it is not. Uncovered by tests, unchanged by the PR: a URL already ending in `?` produces `?&token=` (empty first pair). Pre-existing in both implementations; not this PR's problem. --- ## 3. Tri-state `confirmed_reads` — **correct conditional, but the value mapping has the bug one level down** (should-fix) The conditional you were worried about is right. `init.lua:69` is `if opts.confirmed_reads ~= nil then`, not `if opts.confirmed_reads then`. Measured: ``` confirmed_reads=nil -> ws://h/sub confirmed_reads=false -> ws://h/sub?confirmed=false confirmed_reads=true -> ws://h/sub?confirmed=true ``` `false` is sent, not swallowed. Upstream confirms the tri-state is real: `SubscribeQueryParams.confirmed` is `Option<bool>` (`crates/client-api/src/routes/subscribe.rs:100`). But `init.lua:70` maps the value with `opts.confirmed_reads and "true" or "false"`, so every non-`false` truthy value collapses to `"true"`. Measured: ``` confirmed_reads=0 -> ?confirmed=true confirmed_reads="" -> ?confirmed=true confirmed_reads="false" -> ?confirmed=true <-- exact inversion light="false" -> ?light=true <-- same, init.lua:66 ``` A value arriving from a config file, a `json.decode`, or a Defold `.ini` is a **string**. `confirmed_reads = "false"` requesting `confirmed=true` is the same silent-inversion class the PR body congratulates itself on avoiding for `nil`, just moved from the conditional into the value. On a frozen public API this is an input-validation gap at a trust boundary. **Fix, one line each:** ```lua assert(opts.light == nil or type(opts.light) == "boolean", "stdb: light must be a boolean") assert(opts.confirmed_reads == nil or type(opts.confirmed_reads) == "boolean", "stdb: confirmed_reads must be a boolean or nil") ``` ### Test-honesty note on the `nil` case `test_init.lua:52-56` asserts `subscribe_url(base, { confirmed_reads = nil })`. Lua's table constructor drops nil fields — I counted the fields, it is **0** — so that table *is* `{}` and the assertion is byte-identical to `test_init.lua:34`. It documents intent, which is fine, but the PR body's *"each `confirmed_reads` state including `nil` producing no parameter at all"* over-claims it as distinct coverage. The real `nil` coverage is line 34. Similarly `test_init.lua:70-74` (`subscribe_url(base, {}, "tok") == with_token(base, "tok")`) is self-referential: both sides route through `with_param`, so a change to the separator logic moves both and the test still passes. What actually pins today's bytes is the pair of literal-string assertions at `test_init.lua:17-27`, unmodified by this PR — so the PR's claim is true, just via a different test than it credits. Cheap improvement: assert against the literal `base .. "?token=tok"`. --- ## 4. Parameter escaping — **absence is correct, not latent** (nit: write the reasoning down) No encoding is right here, and I checked the server side rather than assuming: - `light`/`confirmed` values are the hardcoded literals `"true"`/`"false"`. Nothing to encode. - Upstream parses with axum's `Query<SubscribeQueryParams>` (`subscribe.rs:120-127`), i.e. `serde_urlencoded`, so values *are* percent-decoded server-side. - A SpacetimeDB token is a JWT: base64url (`A-Za-z0-9-_`) plus `.` separators. None of those are percent-significant, and base64url never emits `+` or `/`. Decoding is a no-op for exactly the inputs this parameter carries. Your instinct is right. Latent only for a non-JWT `token` containing `&`, `=`, `%`, `+` or a space — which would corrupt the query string. That is pre-existing in `with_token`, not a regression, and encoding it now would change `with_token`'s output for precisely the inputs that are already broken. **Recommendation: do not encode.** Add one comment line to `with_param` (`init.lua:44`) stating that values must already be URL-safe — literals and JWT base64url — so the next person adding a user-supplied parameter through this now-shared helper does not inherit a silent bug. The helper's own docstring currently advertises that *everything* goes through it, which makes the unstated precondition more dangerous than it was when only the token used it. --- ## 5. Ordering claim — **accurate and genuinely pinned** `init.lua:66-74` is three sequential `if` statements, straight-line, no table iteration — had it been a `pairs` loop over a params table, order would have been non-deterministic and unassertable. It is not. Pinned by two tests that would fail on a swap: `test_init.lua:58-62` (`?light=true&confirmed=false`) and `:64-68` (`?light=true&confirmed=true&token=tok`). Claim verified. --- ## 6. Both platform paths — **no bug; verified by running `connect`** (but zero test coverage) This is the one you flagged as most likely broken. It is correct, and the reason is `init.lua:161`: `local url = M.subscribe_url(opts.url, opts, url_token)` sits **outside** the `if token` block, so the URL is composed unconditionally. `url_token` stays `nil` on native, so the token stays out of the URL and `params.headers` is still set at `:158`. I stubbed `_G.websocket` and captured what `websocket.connect` actually receives: | scenario | url | headers | |---|---|---| | native, light+confirmed, no token | `…subscribe?light=true&confirmed=false` | nil | | native, light+confirmed, token | `…subscribe?light=true&confirmed=false` | `Authorization: Bearer JWT.a-b_c` | | html5, light+confirmed, token | `…subscribe?light=true&confirmed=false&token=JWT.a-b_c` | nil | | native, defaults, token | `…subscribe` | `Authorization: Bearer JWT.a-b_c` | | html5, defaults, token | `…subscribe?token=JWT.a-b_c` | nil | | native, defaults, no token | `…subscribe` | nil | Both transports get `light`/`confirmed`; native still authenticates by header with no token in the URL; HTML5 appends the token last; and the default rows are byte-identical to pre-PR output. Also worth noting the URL is recomputed inside `open()` rather than hoisted, so a reconnect does not accumulate duplicate parameters — the other classic shape of this bug is absent too. **The gap is coverage, not correctness.** If someone moved `:161` back inside the `if token` block, **every test in this PR would still pass**, and native builds would silently lose light mode — on the platform the README's bandwidth argument is aimed at. The pure-seam tests structurally cannot see this, because the platform branch lives in `connect`. This is what turns finding 1 from a taste argument into a concrete one: the stubbed-`connect` test both deletes the new public API and covers this. --- ## 7. The README claim — **accurate, verified against upstream, and untested where it matters most** (should-fix) I checked the source rather than trusting the PR body. `crates/core/src/subscription/module_subscription_manager.rs`: > *Regardless, the update that we send to the caller, if we send any, is a full tx update, rather than a light one. That is, in the case of the caller, we don't respect the light setting.* The caller branch sends `TransactionUpdateMessage { event: Some(event.clone()), … }`; the light gate `client.config.tx_update_full.then(|| event.clone())` appears only in the non-caller loop. So **"Your own reducer calls still come back as full updates" is correct**, as is `?light=true` → `tx_update_full: !light` (`subscribe.rs:206`). Why that sentence is load-bearing rather than cosmetic: `handlers.TransactionUpdateLight` (`stdb/connection.lua:324-327`) applies the cache and emits `transaction`, but a light message carries no `request_id`, so unlike `handlers.TransactionUpdate` (`:303-311`) it never clears `c.pending[request_id]`. If a caller's own update ever arrived light, every `db:call_reducer` callback would silently never fire and `c.pending` would leak entries for the life of the connection. The README sentence is the only thing standing between this PR and that failure — and #24 says explicitly: *"every claim about the wire is proven by a captured fixture, not by reading server source. This repo has been burned three times by the wire disagreeing with the docs."* **The deferral is broader than #22 authorised.** #22 offers deferring *"did the server honour light mode"*, which needs a second connection to cause a transaction this client did not call. The bolded README claim needs **one** client: connect with `light = true`, call a reducer yourself, assert the update arrives as full `TransactionUpdate` with `reducer_call` present and the reducer callback fires. That fits the existing single-client harness with no new infrastructure. The PR's *"Deliberately not done"* section is admirably explicit about the half it names, and silently sweeps this half in with it. **Cheaper second gap, independent of any server.** Nothing in `dev/` mentions `TransactionUpdateLight` — zero grep hits across `dev/`, zero in `dev/test/fixtures/session.jsonl`. Before this PR that handler was unreachable for a v1 client; this PR is what makes it reachable. The PR body's *"connection.lua already handles that message, so this adds no new message handling — only the ability to ask"* is technically true and rhetorically misleading: it adds the ability to reach an **entirely untested handler**. Eight lines in `test_connection.lua` using the existing `new_conn()` + `conn:handle_frame(json.encode{ TransactionUpdateLight = { update = … } })` pattern, asserting cache counts and the emitted `transaction` event, closes it. ### Two smaller README accuracy points **`README.md:365-367`** — *"`nil` and `false` are different requests to the server"*. At the wire level yes; behaviourally on the protocol this SDK speaks, no. `resolve_confirmed_reads_default` (`subscribe.rs:103-111`) maps `WsVersion::V1 + None` → `false`, identical to `Some(false)`; upstream's own test pins it (`subscribe.rs:2838-2844`). The tri-state is still the right design — V2/V3 map `None` → `DEFAULT_CONFIRMED_READS`, which is `true` — so the honest justification is **forward compatibility**: `nil` means something different on the next protocol version, and a boolean default would have baked in today's meaning. One sentence, nit. **Informational, no action.** Upstream also computes `hide_reducer_info_for_non_callers = matches!(module_def_version, RawModuleDefVersion::V10)` and sends `event: None` to non-callers regardless of `light`. For V10 modules part of light mode's saving is already unconditional, so the README's bandwidth pitch may overstate the marginal delta on some modules. Doesn't affect the change. --- ## Claim audit | PR body claim | verdict | |---|---| | `with_token` same signature, same output | ✅ byte-identical on all 10 non-erroring inputs; error text differs only | | proven by two pre-existing tests passing unmodified | ✅ `test_init.lua:17-27` untouched, literal expected strings | | order fixed light → confirmed → token | ✅ straight-line code, pinned by two tests | | tri-state `nil` omits the parameter | ✅ `~= nil`, not truthiness | | eight new assertions | ✅ exactly 8 | | "each `confirmed_reads` state including `nil`" | ⚠️ the `nil` assertion is byte-identical to the `{}` one; Lua drops nil fields | | default case byte-identical to today's | ✅ true, but proven by the *old* literal tests, not by `:70-74`, which compares new code to new code | | "adds no new message handling" | ⚠️ true, but it makes a handler with **zero** test coverage reachable for the first time | | "caller always still receives a full update" | ✅ verified in upstream source — but that is exactly the kind of claim #24 forbids resting on source reads | | `5 passed, 0 failed`; stylua clean | ✅ both reproduce | --- ## Does it close #22? **Substantially yes, with two acceptance gaps.** Met: light alone (`:36`), confirmed alone (`:38-42`), both (`:58-62`), both plus token proving `?`/`&` composition (`:64-68`), default byte-identical (via `:17-27`). Both options genuinely reach the URL on both platforms. #22's request to *"say so explicitly rather than leaving it ambiguous"* about the conformance deferral is honoured with a dedicated section. Not met: 1. **The acceptance criterion is only satisfied for the pure seam, not for `connect`.** #22 asks that the options be threaded onto the URL; nothing tests that either platform branch does so. Reverting `init.lua:161` into the `if token` block would break native light mode with a fully green suite. 2. **The deferral is over-broad.** The single-client half of the light-mode claim — the caller still gets a full update — is testable in the existing harness and is now asserted in bold in consumer-facing docs with nothing behind it. `handlers.TransactionUpdateLight` also has no unit test at all. --- ## Summary **Blocker (1):** make `M.subscribe_url` local; test through `connect` with a stubbed `websocket` global (~15 lines, verified working), which also closes gap 6. Do not freeze this signature — it takes an opts table and ignores `opts.token`. **Should-fix (3):** assert boolean-or-nil on `light`/`confirmed_reads` (`"false"` currently requests `true`); add the single-client conformance assertion for the bolded README claim, or soften the README; add a `TransactionUpdateLight` unit test to `test_connection.lua`. **Nits (4):** note the unstated URL-safe precondition in `with_param`; reword the `nil` vs `false` justification as forward compatibility; strengthen `test_init.lua:70-74` to a literal string; the `confirmed_reads = nil` assertion is documentation, not coverage — worth labelling as such. The engineering is sound and the refactor is the right one — `with_param` is exactly the correct shape and the compatibility is genuinely byte-identical. What needs work is the public surface it exposes on the way to a freeze, and the distance between what the README asserts and what any test can detect.

yuris referenced this pull request from a commit 2026-08-19 16:19:33 +00:00

Keep the subscribe URL builder private and prove light mode on the wire

yuris added 1 commit 2026-08-19 16:19:33 +00:00

Keep the subscribe URL builder private and prove light mode on the wire ... 047977e0aa

Review of #33. `M.subscribe_url` was public only because it was the seam the
unit tests drove, and 1.0 (#24) would have frozen it: a function taking a
connect-opts table while ignoring `opts.token`, so the obvious
`stdb.subscribe_url(opts.url, opts)` drops the token and — on HTML5 — connects
successfully as a brand new anonymous identity. It is `local` now, and the URL
tests drive `connect` itself with a stubbed `websocket`/`timer` and a stubbed
`is_html5`.

That swap is what closes the real coverage gap, not a style preference. The
platform branch deciding whether the token joins the query string lives inside
`connect`, so no pure seam could see it: moving the composition back into the
`if token` block silently dropped `light`/`confirmed` on every tokenless native
connection with the whole suite green. Verified by doing exactly that — the
suite now fails with "expected …?light=true, got …/subscribe".

Values must be booleans. `opts.light and "true" or "false"` collapsed every
truthy value, so `light = "false"` out of a config file or a `json.decode`
requested `light=true` — the caller's intent inverted, silently, at a trust
boundary on a frozen API. Two asserts, boolean-or-nil.

The bolded README claim that a caller's own updates stay full is now asserted
against a live server rather than derived from reading upstream source, which
#24 forbids resting on: the headless run connects with `light = true`, calls a
reducer, and fails unless the update arrives as a full `TransactionUpdate`
carrying `reducer_call` and the callback has fired. Load-bearing because a light
update has no `reducer_call.request_id`, so a downgraded caller would see every
`call_reducer` callback never fire and `c.pending` leak for the connection's
life. Confirmed non-vacuous by sending `light=notabool`, which the server
rejects — the parameter is parsed, not ignored.

Also: a unit test for `handlers.TransactionUpdateLight`, which this PR makes
reachable for a v1 client for the first time and which had none; a note that
`with_param` never percent-encodes, now that it is shared rather than the
token's alone; and the tri-state justification reworded to forward
compatibility, since V1 maps absent and `confirmed=false` identically and it is
V2/V3 defaulting to `true` that makes `nil` the right default.

Still deferred, and now the only deferral: proving light mode is honoured for
*other* clients' transactions needs a second connection the harness lacks.

yuris merged commit b52f8ed59a into main 2026-08-19 16:22:17 +00:00

yuris referenced this pull request from a commit 2026-08-19 16:22:18 +00:00

connect: expose light mode and confirmed reads (#33)

yuris referenced this pull request 2026-08-19 18:26:34 +00:00

v1.0.0 release checklist #24

yuris referenced this pull request 2026-08-20 01:45:51 +00:00

v1.0.0 release checklist #24

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:defold

Defold engine integration

  

area:docs

Documentation and examples

  

area:infra

Flake, CI, secretspec

  

area:protocol

Wire protocol conformance

  

area:testing

Test harness and fixtures

  

kind:bug

Something broken

  

kind:chore

Maintenance

  

phase:breadth

UnsubscribeMulti, procedures, BSATN

  

phase:core

Core SDK layers (protocol/cache/connection)

  

phase:hardening

Reconnect, token persistence, errors

  

phase:slice

Vertical slice: first playable end-to-end

No labels

area:defold

area:docs

area:infra

area:protocol

area:testing

kind:bug

kind:chore

phase:breadth

phase:core

phase:hardening

phase:slice

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

yuris/spacetimedb-defold!33

No description provided.