Skip to content

fix(relay): an iPad no longer gets a Done alert for every unrelated publish - #15015

Open
juliusmarminge wants to merge 4 commits into
mainfrom
t3code/relay-push-spam
Open

juliusmarminge wants to merge 4 commits into
mainfrom
t3code/relay-push-spam

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

iPads and iPhones using regular push notifications could receive the same Done alert whenever an unrelated thread published or a subagent was removed. The fallback selected the first row of the user’s activity aggregate, which could still contain a different thread’s recent completion.

Build fallback alerts from the published thread’s state. The publisher now uses one APNs delivery path, carrying that state separately from the Live Activity aggregate. Tombstones and unrelated running updates stay silent; an unrelated thread needing input gets its own text and deep link. Notification-only environment links still work, and their notification setting gates iOS fallback pushes. Existing aggregate Live Activity selection, replay, delivery-time checks, and Android behavior are preserved.

Maintainer takeover of the original work by @juliusmarminge in #15015. Retained the original history, merged current main, replaced argument-only tests with publisher-to-signed-queue coverage, and removed the parallel notification-only aggregate path and redundant comments.

Verification on headless Linux:

  • Before → after: 111 → 111 passing tests across ApnsDeliveries.test.ts (51), AgentActivityPublisher.test.ts (16), and FcmDeliveries.test.ts (44). Current main has 110. The original already added only one net test, so the cleanup strengthens observable coverage without growing the suite.
  • Regression proof: with main’s two production files, the selected three cases fail: four stale Done alerts are queued, and link opt-out still queues alerts. Restoring this implementation makes all three pass. The regression also checks an unrelated input notification’s identity, body, and deep link.
  • pnpm exec vp test run infra/relay/src/agentActivity/{ApnsDeliveries,AgentActivityPublisher,FcmDeliveries}.test.ts: 111 passed.
  • Relay-only typecheck and lint/formatting on the four changed files passed.
  • Independent whole-PR adversarial review: GPT-6-Sol and Claude Opus 5.5, round 1. Both found no correctness blockers; optional typing and delivery-count logging suggestions were left out of this focused fix.

This is nonvisual relay behavior; the signed-queue integration tests provide before/after proof. No production relay was deployed and no live APNs, physical iOS/iPadOS, Android, macOS, Windows, or Electron desktop run was performed. A relay deploy and device smoke test remain for a human. This does not deduplicate repeated publication of the same completed state or change aggregate Live Activity alert policy.

Implemented and tested with GPT-6.1-Sol and GPT-6-Astra through the Codex harness in T3 Code; independently reviewed by GPT-6-Sol (Codex) and Claude Opus 5.5 (Claude Agent).

Fixes #28932

…ublish

Devices without a Live Activity token (iPads, iPhones with Live Activities
off) fall back to regular push notifications. That fallback alerted on the
first row of the user's aggregate rather than the thread being published.
A finished thread stays at the top of the aggregate for two minutes, so
every publish in that window re-sent its Done alert: one completion with six
subagent tombstones produced 42 pushes in a minute.

Push alerts now describe only the published thread's state, matching the
Android path. The notification-only send folds into sendForTarget so one
function decides whether an iOS device rings.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 3, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 3, 2026
@github-actions github-actions Bot added the size:M 30-99 changed lines (additions + deletions). label Oct 3, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 24406cf

Macroscope's review found this PR approvable — Fallback Apple notifications now use the currently published thread instead of potentially unrelated aggregate activity. Existing notification preferences, freshness checks, and replay suppression remain in place, keeping the change narrowly scoped.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 4.9 KiB 4.9 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.7 KiB 3.7 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.1 KiB 1.1 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.4 KiB 20.4 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 1 1 0 (0.0%) 8 ✅
Claude Total thread wire 4.9 KiB 4.9 KiB −41 B (−0.8%) 6.8 KiB ✅
Claude Thread snapshot wire 3.7 KiB 3.7 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB −41 B (−3.4%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 20.8 KiB 20.7 KiB −41 B (−0.2%) 29.3 KiB ✅
Claude Live turn messages 2 1 −1 (−50.0%) 8 ✅

Baseline: 22e9d35 · PR result: 1a7f6e7 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 106.1 KiB
  • Claude decoded thread snapshot: 106.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

APNs fallback notifications now use the published agent activity state instead of selecting an activity from the aggregate. AgentActivityPublisher passes that state through one APNs delivery call. Android delivery remains unchanged.

Changes: State-Based APNs Delivery

Layer / File(s) Summary
Select and enqueue notifications from thread state
infra/relay/src/agentActivity/ApnsDeliveries.ts, infra/relay/src/agentActivity/ApnsDeliveries.test.ts
sendForTarget accepts optional thread state. Fallback notifications use that state and skip missing, expired, or non-alertable states. The separate push-notification operation is removed. Tests cover state-based selection and alert payloads.
Pass published state through publisher delivery
infra/relay/src/agentActivity/AgentActivityPublisher.ts, infra/relay/src/agentActivity/AgentActivityPublisher.test.ts
The publisher uses one APNs sendForTarget call and passes state only when notifications are enabled. Tests check queued notification content and the disabled-notification case. Android delivery remains unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

  • pingdotgg/t3code#25582: This change updates the notification-only fallback path, but does not add a phase-transition guard for title-only republishing.
  • pingdotgg/t3code#11938: This change uses published state for fallback notifications, but does not deduplicate repeated completed states.

Suggested reviewers: ryanrhughes, t3dotgg

Merge Risk: Minimal · up to 24406

This change stops unrelated publishes from producing Done alerts on iOS, and unrelated threads that need input now get their own alert. No merge-blocking risk was found. Deployment and a device smoke test are still pending, as the author notes.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary fix: preventing unrelated publishes from triggering repeated Done alerts on iPads.
Description check Passed The description covers the problem, implementation, scope, linked issue, verification results, limitations, and human follow-up. It provides specific tests and observed outcomes instead of only statin…
Linked Issues check Passed Issue #28932 requires fallback alerts to use the published thread, remain silent for starting/running states, honor completion and attention settings, and avoid alerts when notifications are disabled.…
Out of Scope Changes check Passed The changed production code stays within issue #28932. It replaces the notification-only aggregate path with state-based fallback selection and updates the shared APNs delivery path. The changed tests…
Full details: Description check

Explanation

The description covers the problem, implementation, scope, linked issue, verification results, limitations, and human follow-up. It provides specific tests and observed outcomes instead of only stating that tests passed.

Full details: Linked Issues check

Explanation

Issue #28932 requires fallback alerts to use the published thread, remain silent for starting/running states, honor completion and attention settings, and avoid alerts when notifications are disabled. AgentActivityPublisher.ts passes the published state separately from the Live Activity aggregate. ApnsDeliveries.ts builds fallback payloads from that state and applies shouldAlertForActivity, expiry checks, and notification preferences. The tests cover stale aggregate rows, unrelated running and waiting states, input and approval alerts, notification-only links, and notification opt-out. Aggregate Live Activity and Android paths remain separate.

Full details: Out of Scope Changes check

Explanation

The changed production code stays within issue #28932. It replaces the notification-only aggregate path with state-based fallback selection and updates the shared APNs delivery path. The changed tests provide publisher-to-queue and APNs regression coverage for that behavior. No unrelated product behavior is demonstrated.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 1a7f6e7 and 24406cf.

Files selected for processing: 4

  • infra/relay/src/agentActivity/AgentActivityPublisher.test.ts
  • infra/relay/src/agentActivity/AgentActivityPublisher.ts
  • infra/relay/src/agentActivity/ApnsDeliveries.test.ts
  • infra/relay/src/agentActivity/ApnsDeliveries.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@juliusmarminge

Copy link
Copy Markdown
Member Author

Taking over this one as maintainer for batch ssh-relay-remote-12. I’ll retain the original branch and author history, merge current main, simplify the regression coverage, and run independent implementation and adversarial review passes. No merge is planned.

@macroscopeapp
macroscopeapp Bot dismissed their stale review October 19, 2026 06:18

Dismissing prior approval to re-evaluate 9d46a6d

# Conflicts:
#	infra/relay/src/agentActivity/AgentActivityPublisher.test.ts
#	infra/relay/src/agentActivity/ApnsDeliveries.test.ts
#	infra/relay/src/agentActivity/ApnsDeliveries.ts
},
);

it.effect("push-only alerts describe the published thread, never a stale aggregate row", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This repeats the push-only delivery setup in the preceding test. Could you fold these stale-aggregate cases into that existing test instead of adding a separate it?

Posted via Macroscope — Test Signal

@juliusmarminge

Copy link
Copy Markdown
Member Author

The automated pipeline stopped on this pull request and will not merge it:

the proof has no capture anyone can open:

  • Repeated Input and Done alerts on base were not observed.
  • Suppression of unrelated running updates and state clears on head was not verified.
  • Eligible notifications' text and deep links, and disabled-environment notification suppression, remain unverified.
  • Real proof requires isolated authenticated relays running both commits, valid APNs credentials, and a registered APNs-capable iOS device. The scenario fixture's direct pairing does not exercise the hosted relay.

It stays open for a person to finish or close.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Every agent-activity publish queues a push while any thread waits for input (burst of identical "Input:" pushes)

1 participant