Repository navigation
Conversation
4 failing -> 0 failing (compile failure -> 154 passing)
…s-mcp-tool' into issue-126-add-registerevent-to-the-swift-sdk
V3RON
left a comment
There was a problem hiding this comment.
Verdict: request changes (posted as a comment because GitHub blocks request-changes on your own PR). 1 blocker, 1 should-fix, 1 nit. Spec: issue #126 plus the #95 design comment.
Fix first: testFramesMatchTheSharedFixture is flaky because the SDK doesn't order the post-ack snapshot before later deltas, and the shared fixture requires that order. Decide which side gives (SDK or harness) before Kotlin (#146) adopts the fixture.
| try await connect(client, transport, sessionId: sessionId, eventRegistry: true) | ||
|
|
||
| let expected = (scenario["frames"] as! [Any]).map { JSONValue.from(foundation: $0) } | ||
| for step in scenario["afterAck"] as! [[String: Any]] { |
There was a problem hiding this comment.
Blocker: this test is flaky; the SDK doesn't guarantee the frame order the fixture pins. connect returns as soon as onAckReceived has scheduled the snapshot Task. The afterAck steps then run right away, racing it: sendEventSnapshot reads eventStore whenever it gets to run (after the tool snapshot's await sendWire), and the delta Task can reach the actor first. I ran swift test --filter AppductEventRegistryTests 25 times on the PR head and this test failed 9 times with every bad ordering:
[upsert cart.item_added, snapshot[cart.item_added]][snapshot[cart.item_added], upsert][remove checkout_completed, snapshot[]][snapshot[], remove]
The daemon ends up in the right state each time, so the bug is in the contract, and Kotlin is about to adopt it. Fix one side or the other: either make the SDK capture the snapshot at ack time and send later deltas after it, or have the harness wait for the event_registry_snapshot frame before applying afterAck and say so in fixtures/README.md.
| `event_registry_snapshot` / `event_registry_delta` frames (`docs/PROTOCOL.md` §5a) an SDK sends. | ||
| Each case is a scenario run through the SDK's public API against a fake transport: declare every | ||
| descriptor in `declaredBeforeAck` (valid `EventDescriptor`s in wire form), connect, deliver a | ||
| `session_ack` that carries `"event_registry": true`, then apply each `afterAck` step in order |
There was a problem hiding this comment.
Part of the blocker on the test: "deliver a session_ack ..., then apply each afterAck step" gives no sync point. Yet frames requires the snapshot to show exactly declaredBeforeAck and to come before every delta. A Kotlin harness written to this text will hit the same race. Name the sync point here (for example "once the event_registry_snapshot frame has been sent"), or state that the SDK must order them.
| Against an older `appduct` CLI that predates event lists, the app keeps its session and tools and | ||
| `appduct events ls` shows nothing. | ||
|
|
||
| Read back with `appduct events tail`. Throws (does not send) unless a session is currently active. |
There was a problem hiding this comment.
Should-fix: this sentence is about postEvent, but the new block now sits between them. A reader takes "Throws (does not send) unless a session is currently active" to mean registerEvent only works during a session. They will defer declarations until after connecting, when declaring before is exactly what works. Move this sentence back under the postEvent snippet, ahead of the declaration paragraph.
| let name = descriptor.name | ||
| return EventRegistration { [weak self] in | ||
| guard let self, self.eventStore.remove(name) else { return } | ||
| Task { await self.sendEventRegistryDelta(.remove(name)) } |
There was a problem hiding this comment.
Nit (unverified): remove() then an immediate registerEvent of the same name spawns two unstructured Tasks from a nonisolated context, and nothing orders their hops onto the actor. If the upsert lands first, the daemon ends up without an event the app still declares, until the next resume. registerTool/unregisterTool have the same pattern. A stress test that removes and re-registers in a loop and checks the last delta would confirm it.
Snapshot at ack time first, then deltas in call order; 9/25 fixture runs failing -> 0/40
…s-mcp-tool' into issue-126-add-registerevent-to-the-swift-sdk
V3RON
left a comment
There was a problem hiding this comment.
Approve, round 2: 0 blocker, 0 should-fix, 0 nit. Spec: issue #126.
The single ordered queue fixes the snapshot/delta race and the remove-then-register race; event registry and fixture tests passed 150/150 runs (90 under 6x parallel load), full Swift suite green, and the fixtures/README ordering contract is one Kotlin can meet with a lock plus one ordered sender.
…s-mcp-tool' into issue-126-add-registerevent-to-the-swift-sdk
…s-mcp-tool' into issue-126-add-registerevent-to-the-swift-sdk # Conflicts: # CHANGELOG.md
…s-mcp-tool' into issue-126-add-registerevent-to-the-swift-sdk
…s-mcp-tool' into issue-126-add-registerevent-to-the-swift-sdk
Closes #126
Stacked on #143 (issue #124); base is that branch.
What changed
Swift apps can declare events with
Appduct.shared.registerEvent(...)(core:AppductClient.registerEvent). The SDK sends a snapshot after every ack carryingevent_registry: true, resume included, then upsert and remove deltas, and nothing against an older CLI. Addsfixtures/event-registry-frames.jsonfor the Kotlin slice.Acceptance criteria
fixtures/event-descriptors.jsonFixturesConformanceTests.testEventDescriptorsFixtureevent_registry: true, resume included, is followed by a snapshotAppductEventRegistryTests.testEveryAckCarryingTheFlagIsFollowedByASnapshotIncludingAfterResumeremovedeltatestFramesMatchTheSharedFixture(disposing case),testRemovingTwiceSendsOneRemoveDeltatestAckWithoutTheFlagSendsNoEventFramestestFramesMatchTheSharedFixtureover newfixtures/event-registry-frames.jsonE2E evidence
Target: iOS simulator (iPhone 17 Pro, iOS 26.4),
playground-native/ios, commit 847d462. Isolated daemon (APPDUCT_STATE_DIR, port 47213). TworegisterEventcalls added locally toPlaygroundTools.registerAll(), not committed.Smoke: SMOKE_OK (
throwing_toolreturnedtool_execution_error, exit 72)Feature:
Resume: app backgrounded (session
suspended,app_backgrounded), foregrounded, sessionactiveagain;events lslisted both events,--name cart.clearedand--jsonstill worked.Observation: property order in the signature is not stable across runs (
{ total?, orderId }before,{ orderId, total? }after resume), becausepayloadSchemagoes through an unordered Swift dictionary.Checklist
CHANGELOG.mdhas an entry underUnreleased(writing-changelogskill), or the change is not user-visiblewriting-user-docsskill), or the change is not user-visibleindex.ts; no new directnode:*I/O outside an adapterarchitectureskill applied, exceptions explained abovedocs/ARCHITECTURE.mdupdated if a surface it describes changedOut of scope
I edited one #124 line: the fixtures README sentence saying event-descriptors.json is TS-only.
AppductRegistryDeltabecame generic over the descriptor (tools and events are its two callers). Dev warnings stay React Native only.No tool registry frames fixture existed, so
event-registry-frames.jsondefines the scenario shape, documented inpackages/native/fixtures/README.md, for the Kotlin slice (#127) to reuse.Status
Implement: done (5/5 green) Review: round 2, approve E2E: pass (iOS, playground-native) Ready: yes