Repository navigation
fix(ios): post an event's ts in unix milliseconds (#154) - #158
Conversation
Fails on the current core: the frame carries 1752600000 where the protocol says 1752600000000. 1 failing
timers.now() is already milliseconds, so the extra / 1_000 put every iOS event 1000x in the past. The vendored @appduct/react-native copy regenerates from this file at build time. 1 failing -> 0 failing
…report-a-timestamp-in-seconds # Conflicts: # CHANGELOG.md
|
|
||
| /// Emits an `event` frame while active (PROTOCOL.md §7); rejects otherwise. | ||
| /// Emits an `event` frame while active (PROTOCOL.md §4); rejects otherwise. `ts` is Unix | ||
| /// milliseconds, the same unit `timers.now()` reports and Kotlin's `System.currentTimeMillis()` |
There was a problem hiding this comment.
should-fix — the unit this fix establishes is stated in words nowhere a user reads, and criterion 2 was checked against a deleted file.
docs/PROTOCOL.md §4 documents ts only as the literal 1752600000000 — the guard sentence right below it explains the frame's plumbing but never says "milliseconds". The one sentence that ever did, "ts is in milliseconds", lived on website/src/content/docs/reference/protocol.md:182 — the second path criterion 2 cites. #152 deleted that file (2e587bd, an ancestor of this head), so it is not in this tree, the docs-site page 404s today, and llms.txt lists no protocol page under Reference. That leaves the unit carried by a 13-digit number and a Swift doc comment.
Concrete cost: the contract's own header says the docs are what a new client implementation reads, and #154 is what happens when one SDK guesses. The bug shipped from #49 through 0.14.0 with Android right and iOS wrong because nothing readable pinned the unit; after this PR, still nothing readable does.
Fix direction: state it in §4's event paragraph next to the guard sentence ("ts is Unix milliseconds") — one sentence, and it makes the shipped value and the contract agree in prose, not just by example. The PR body's criterion-2 evidence also needs correcting, since one of its two paths no longer exists.
There was a problem hiding this comment.
Confirmed the deletion (git log --diff-filter=D -- website/src/content/docs/reference/protocol.md -> 2e587bd) and that no live page covers the unit: nothing under website/src/content/docs states a unit for an event's ts, and the only "millisecond" left in the website's prose is writing-tools' parameter advice. Agreed, so 93d2f07 says it in words in three places rather than one:
docs/PROTOCOL.md§4, next to the guard sentence, as you suggested — plus that the guard accepts any finite number, so a seconds stamp is not a validation error but a silent 1 000x-past timestamp. That is the part §4 was missing for the next client implementer, not just the unit.website/src/content/docs/reference/cli.md,events tail: each line'sdata.tsis the app's stamp in Unix milliseconds, and the time leading the line is the daemon's receive time. I went to the website too because docs: shorten README, add banner and remove stale docs #152 moveddocs/out of thewriting-user-docsskill's user-facing list, so §4 alone would have been the contributor contract saying what the user docs still didn't — the same gap that let this ship.skills/appduct/references/cli.md, same command: an agent writingevents tail --jsonis the reader most likely to parse the wrongts, so the line spells out which of the two timestamps is the app's.
Also corrected: criterion 2's evidence line now cites these three, and the changelog says data.ts where it said ts, since that is the key it publishes.
|
|
||
| let text = try XCTUnwrap(transport.sentMessages.first { $0.contains("\"type\":\"event\"") }) | ||
| let frame = try JSONValue.parse(text) | ||
| XCTAssertEqual(frame.objectValue?["ts"]?.doubleValue, Double(nowMs)) |
There was a problem hiding this comment.
nit — this pins the arithmetic, not the whole chain, so one link is still assumed: no test anywhere asserts that SystemAppductClientTimers.now() actually returns milliseconds (grep -rn SystemAppductClientTimers packages/native/ios/Tests is empty).
State that slips through: drop or double the * 1_000 in AppductClientTimers.swift:31. This test stays green — it injects FakeClientTimers, so it only proves postEvent forwards the port's number unscaled — and iOS is back to a 1970 data.ts plus grace/backoff windows 1000x off. That adapter also exists twice in the tree (packages/native/ios/.../Real/ plus the build-time vendored copy in packages/react-native/ios/Core/), so drift is a two-file affair rather than one.
Cheap close: one test in AppductClientTests (or a small AppductClientTimersTests) asserting SystemAppductClientTimers().now() is the same magnitude as Date().timeIntervalSince1970 * 1_000 — the repo already tests real adapters this way (SPKI pin, lease store). The tier choice itself looks right to me: I confirmed the fixture arguments — packages/native/fixtures has no event-frame vectors, and Kotlin has no clock seam to fake (System.currentTimeMillis() inline in all 13 sites, no Clock interface).
There was a problem hiding this comment.
Fair — the fake made criterion 1 prove only forwarding. Added c621870: AppductClientTimersTests.testTheRealClockReportsUnixMilliseconds asserts SystemAppductClientTimers().now() equals Date().timeIntervalSince1970 * 1_000 within 1 s, which is the adapter's own definition rather than a magnitude band, so a seconds or microseconds clock is out by three orders and fails. Verified your mutation: * 1_000 dropped, the new test fails with 1791298761 against 1791298761603 and the postEvent test stays green, which is exactly the hole you described. Restored, 157 tests 0 failures.
No #if APPDUCT_ENABLED guard, following AppductAPITests, which covers the same Real/ directory ungarded: the define is a build setting on the AppductCore target only, and swift test builds that target in Debug, so the test is not silently skippable. Say the word if you would rather have it guarded.
On the second copy: it is not committed, so drift is a one-file affair. packages/react-native/scripts/sync-native-core.mjs (note: under packages/react-native/, not the root scripts/) replaces ios/Core/ wholesale from packages/native/ios/Sources/AppductCore/Real on that package's build and prepack; ios/Core/ is gitignored and git ls-files packages/react-native/ios returns only the bridge's two files, so Core/AppductClient.swift exists here solely because pnpm build regenerated it. I checked android/core and android/core-noop the same way, and git grep --no-index 'public actor AppductClient' finds only the source and that regenerated copy. Deleted ios/Core/ before the final pnpm build to confirm it comes back with the fix; the PR body now says this instead of just asserting it.
V3RON
left a comment
There was a problem hiding this comment.
Comment — 0 blockers, 1 should-fix, 1 nit. The fix is correct and complete: data.ts is now Unix milliseconds on iOS, matching Android and docs/PROTOCOL.md §4; I checked every other timers.now()/currentTimeMillis site in both native cores and the RN JS layer for the same unit bug and found none, and no daemon/CLI/MCP consumer of data.ts assumed seconds (ordering, cursors and retention all run off seq).
Spec: issue #154 plus the PR body's criteria table.
Fix first: the unit is now correct in code but stated in words nowhere a user reads — website/.../protocol.md:182, cited as criterion 2's evidence, was deleted by #152 before this head, so say "ts is Unix milliseconds" in docs/PROTOCOL.md §4 and correct that criterion.
Review nit: every AppductClient test injects FakeClientTimers, so dropping the * 1_000 in SystemAppductClientTimers.now() left the suite green. Verified by mutation: with the multiplier removed this test fails (1791298761 vs 1791298761603) and 156 -> 157 with 1 failing. swift test: 157 tests, 0 failures.
Review should-fix: #152 deleted website/src/content/docs/reference/protocol.md, which held the only sentence ever written about the unit. docs/PROTOCOL.md 4 now states it, and the two surfaces a user or agent reads for this output do too: the website's events tail reference and the shipped skill's CLI table.
Closes #154
What changed
An event posted from an iOS app now reports its time in milliseconds, the unit
docs/PROTOCOL.md§4 documents and the only one Android sends. The iOS core dividedtimers.now()by 1 000 on the way onto the wire even though that clock already reports milliseconds, so the app's own timestamp was 1 000× in the past and showed as a 1970 date whereverdata.tsis printed. One division removed, plus a Swift test that pins the frame'ststo the fake clock's milliseconds so the unit is now specified rather than assumed. The daemon, CLI, MCP surface and the Kotlin core are untouched;@appduct/react-nativepicks the fix up by regenerating its vendored copy of this file at build time —packages/react-native/scripts/sync-native-core.mjscopiespackages/native/ios/Sources/AppductCore/Realintoios/Core/(gitignored; the only tracked files there are the bridge's own two), and deletingios/Core/beforepnpm buildregenerates it with the fix.Acceptance criteria
eventframe an iOS app posts while active carriestsin Unix milliseconds, asdocs/PROTOCOL.md§4 documents and Android sendsAppductClientTests.testPostEventStampsTheEventWithUnixMillisecondsNotSeconds— an active client onFakeClientTimers(startMs: 1_752_600_000_000)posts an event, and the frame the fake transport received hasts == 1752600000000CHANGELOG.mdunderUnreleased;docs/PROTOCOL.md§4 ("tsis Unix milliseconds");website/src/content/docs/reference/cli.md,appduct events tail;skills/appduct/references/cli.md, same commandCriterion 2 is a doc/changelog surface, not a runtime assertion; criterion 1 was red before the fix (the frame carried
1752600000) and is green after.swift testat the repo root: 157 tests, 0 failures.pnpm build,pnpm lint,pnpm typecheckclean;pnpm test923 passed / 1 skipped inappduct, 262 passed in@appduct/react-native;pnpm check:linksclean.Criterion 2's first evidence line was wrong when this PR was opened, and the review caught it: it cited
website/src/content/docs/reference/protocol.md, which #152 deleted before this head, so no live page stated the unit at all — only the 13-digit literal indocs/PROTOCOL.mdand a Swift doc comment did. #152 also moveddocs/out of thewriting-user-docsskill's user-facing list, so §4 alone would not have closed it either. The unit is now written out in §4 for anyone implementing a client, and on the two surfaces a user or agent actually reads for this output: the website'sevents tailreference (itsdata.ts, in Unix milliseconds, distinguished from the daemon's receive time leading the line) and the same command in the shipped skill's CLI table. The changelog entry now namesdata.tsrather thants, since that is the key it publishes.Review follow-ups
SystemAppductClientTimers.now()itself returns milliseconds — everyAppductClienttest injectsFakeClientTimers, whosestartMsis a free parameter.AppductClientTimersTests.testTheRealClockReportsUnixMillisecondsnow pins the production adapter, as the SPKI pin and lease store already pin theirs. Checked by mutation: dropping the* 1_000fails it (1791298761against1791298761603), which the old test did not.Why a Swift unit test and not a conformance fixture
packages/native/fixtureshas noevent-frame vectors today —event-registry-frames.jsoncovers onlyevent_registry_*— and its scenario shape has no way to pin a timestamp: the Kotlin core has no clock seam at all (System.currentTimeMillis()inline, no interface to fake), so a fixture asserting a fixedtscould not run in Kotlin, and a loose one (ts > 1e12) would re-assert a real wall clock rather than the unit. That directory's stated rule is for parsing/validation behaviour three implementations share; this is one SDK's stamping of a field, so the SDK's own test through the public API is the lowest tier that observes it.E2E evidence
pending — left for the
e2e-deviceskill run. Needs an iOS simulator:appduct sessions link --open ios-sim, thenappduct events tail --jsonand tap the playground's event button; the line'sdata.tsshould be ~1.75e12, matching Android.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
ts(the leading time on aevents tailline, and the flattsappduct_events/appduct_wait_for_event/AppClient.waitForEventreturn) is stamped from the daemon's clock and was never wrong; the issue described that value as showing 1970. Only the app-supplieddata.tswas. If callers should see the app's clock rather than the daemon's, that is a separate design question. The two surfaces now say the two values are separate, since the names invite mixing them.AppductClientTest.kt:808asserts only that an event frame has atskey, so nothing pins Android's unit either; worth a follow-up.AppductClient+Session.swiftandAppductConnectionManager.swiftalso dividetimers.now()by 1 000, correctly — bootstrapexpiresAtis Unix seconds. Left alone.packages/appduct/src/mcp/events-tool.tsdescribesappduct_events' flattswithout a unit. Left alone: that value is the daemon's, its unit isEventNotification.ts's "Unix ms", and iOS events report a timestamp in seconds, not milliseconds #154 did not change it.Status
Implement: pending Review: pending E2E: pending Ready: no