feat(push): report push capability so clients stop offering a feature that cannot work - #521
Open
RonenMars wants to merge 2 commits into
Open
feat(push): report push capability so clients stop offering a feature that cannot work#521RonenMars wants to merge 2 commits into
RonenMars wants to merge 2 commits into
Conversation
… that cannot work
RonenMars
added a commit
that referenced
this pull request
Aug 11, 2026
Closes #528. The streamer could send no ordinary notification at all. Mobile registered an Expo token, `POST /api/push/register` stored it, and nothing consumed it except `LiveActivitySender` — so the one moment the away-from-desk workflow depends on, the agent finishing its turn, went unserved. ## Why Expo rather than the existing APNs path `ApnsClient` signs with an APNs `.p8` for `${bundleId}.push-type.liveactivity`, and Apple issues those keys per developer team. A self-hosted streamer's key can never sign for the published app's bundle id, so anything built on that path works for the maintainer and nobody else. Expo already holds the app's APNs and FCM credentials and mobile already registers an Expo token, so a plain batched POST to `https://exp.host/--/api/v2/push/send` needs no Apple credential and covers iOS and Android in one path. `ApnsClient` and the Live Activity path are untouched; this is additive. ## What it does `ExpoPushSender` fans out over `PushRepository.listDeliverable()`, which is already Expo-only, so an ActivityKit token can never reach the relay. Expo returns one ticket per token in a single response, so sends are independent by construction — a dead device is recorded against its own row and never silences the others. A `DeviceNotRegistered` ticket revokes the token, mirroring how `LiveActivitySender` expires a dead APNs one; every other ticket error only counts toward the existing failure streak. `WaitingInputNotifier` hooks the same `onStatusChange` funnel `LiveActivityNotifier` uses rather than adding a detector, so all three `markReady` call sites and the Codex runner are covered by one hook. A turn opens on `waiting_input → running` and the notification fires on the matching `running → waiting_input`, so boot and resume ready notify nothing — you are never told about the session you just started. ## Three things worth a reviewer's attention **The Expo enhanced-security footgun.** Sends are unauthenticated by default, with `THREADBASE_EXPO_ACCESS_TOKEN` as an opt-in `Authorization: Bearer`. Requiring a token would lock out self-hosters, who do not own the Expo project and cannot obtain one — and for the same reason, **enabling enhanced security in the Expo project dashboard would break every self-hosted streamer at once**. That is a switch in a web UI with no representation in this repo, which is why it is written down here and in the guide. **The suppression trade-off.** No push goes out while any live WebSocket client is subscribed to that session, which is the available signal for "the user is already looking" — mobile subscribes when the session screen opens and the socket dies on backgrounding. It errs toward silence in one case: a desktop client left subscribed to a session suppresses the phone's notification too. The alternative needs per-device foreground state that mobile does not report today. **The payload is a privacy decision.** `title: <projectName>`, `body: "Waiting for your input"`, `data: { sessionId, serverId }` — the two ids are exactly what mobile's `sessionRouteFromNotificationData` needs to route a tap, so tapping opens the session with no tb-mobile change. Deliberately absent are `lastOutput` (raw PTY output, so any session printing a token would put it in a notification) and `sessionName` (derived from the user's first message). Carrying both is the divergence RonenMars/threadbase-mobile#636 is open about on the Live Activity path, and this must not repeat it. The test seeds the fixture with a fake secret in `lastOutput` and a prompt-derived `sessionName`, then asserts the serialised request contains neither — a leak test rather than a shape test, so widening the payload later fails CI rather than a policy review. ## Overlap with #521 That PR reports `notifications: false` as a hardcoded capability, reasoned from "`listDeliverable()` has no caller". This PR gives it one, so whichever of the two lands second has to flip that field — it is no longer true that ordinary push cannot work. Cross-linked rather than merged into one change, since the capability surface and the sender are independently reviewable. ## Verification `npm run lint`, `npm run build`, and the full vitest suite were run on this branch. The 15 new tests cover send-on-ready, no double-send within a turn, no send at boot ready, suppression while watched, dead-token eviction, one token failing without blocking the rest, and the payload leak assertions. Each negative assertion was mutation-checked: disabling suppression, removing the per-turn guard, adding `lastOutput` to the payload, and removing the eviction each turn the relevant tests red. The nine files failing locally (`server.close()` hook timeouts in `cors-middleware`, `discovery-cache`, `pair-endpoints`, `security-hardening`, `watch-for-jsonl`, `webhook-update`, `codex-*`) fail identically with this change stashed, at the same durations, and are the dirty-box artifact documented in `docs/testing/cross-platform-ci.md`.
This was referenced Aug 12, 2026
The HTTP block asserted Live Activity capability from APNs credentials alone, which was true when the test was written and stopped being true once the feature moved behind the liveActivityPush flag. It also never cleared THREADBASE_FEATURE_LIVE_ACTIVITY_PUSH, which outranks the config rung, so an exported var in a developer's shell decided the result instead of the test. State the flag per case and clear the env var at boot; each block's afterEach already restores the whole environment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #519.
The server decides at boot whether Live Activity push can work and logs it (
live_activity.enabled/live_activity.disabled), but that decision never reached a client. So mobile offered a feature that silently no-ops for every user who is not the maintainer — the APNs.p8is a server-side maintainer-owned signing secret bound to the app's bundle id.A feature that is absent is fine. A feature that is present, enabled in the UI, and silently does nothing is not.
What changed
GET /api/infoandGET /api/push/healthboth carry an additivepushobject:liveActivitycomes from the server's own wiring state (liveActivityNotifier !== null) rather than a re-read of the environment, because credentials alone do not enable it — the sender is only built when the push token store opened too.notificationsis a hardcodedfalse, reported rather than omitted precisely so a client cannot infer ordinary push works because Live Activity push is configured:expo-server-sdkis not a dependency andPushRepository.listDeliverable()has no caller.liveActivityReasonnames the missing environment variable, never its value — one test locks that down. The half-configured case (APNS_KEYset, team/bundle missing) is the hardest to diagnose, so it names them explicitly.availablewas deliberately NOT changedavailableon/api/push/healthreports whether the SQLite token store opened, not whether credentials exist — which reads like the bug. It was left alone because the released app renders it verbatim:tb-mobile/app/notification-health.tsxmaps it to "Push store is available / unavailable (registration cannot persist)".Retargeting it at credentials would make every non-maintainer server tell its user that registrations cannot persist — false, since the repository is open and
register()works — and would point debugging at the database instead of the missing.p8.parsePushHealthResponseonly type-checks it as a boolean, so the wrong sentence would render with no parse error. That trades one lie for another.The new
pushobject sits alongside it. On/api/push/healththe pairing of"available": truewith"liveActivity": falseis the honest statement of the situation.Backward compatibility
Purely additive.
parsePushHealthResponsebuilds its result explicitly and ignores unknown keys, so released clients are byte-for-byte unaffected. A client that does not knowpushmust read its absence as unknown, not unavailable.Needs a paired tb-mobile issue: mobile should hide or disable the Live Activity affordance when
push.liveActivityis false rather than registering tokens nothing will ever send to.Verification
npm run lintexits 0. 67 push tests and 171 server/capabilities/http-log tests pass.Positive control: forcing the wiring to a constant in each direction turned exactly the opposite two HTTP tests red (
liveActivityPushEnabled: () => true→ both "without credentials" cases;() => false→ both "with credentials" cases). The tests read the server's real state, not a constant.Note
docs/compatibility/tb-mobile.mddid not list the push endpoints at all, despite mobile having shipped consumers for both. Closed in this PR.