Conversation
`allowedCapabilities`/`allowedCapabilityKeys` are model-authored tool arguments,
and nothing in the package narrowed them, so a host that piped tool JSON straight
into `executeJavaScript` — the README Quick Start shape — granted whatever the
script declared for itself.
Adds `CodeModeConfiguration.capabilityGrant` (`CapabilityGrant`), intersected
with the request before the invocation context is built, so the effective set is
always requested ∩ granted. Defaults to `.unrestricted` for source compatibility.
Withheld identifiers produce a `CAPABILITY_WITHHELD_BY_HOST` diagnostic, and the
resulting `CAPABILITY_DENIED` tells the model the denial is not repairable by
widening its own allowlist, so it stops escalating.
Also closes the aliasing hole: `allowedCapabilityKeys` accepts arbitrary strings
and is not validated against `CapabilityID` at decode time, but `invokeBuiltIn`
honoured it and `registeredFunction(for: key)` resolved built-ins first — so
`{"allowedCapabilities": [], "allowedCapabilityKeys": ["keychain.read"]}` reached
the built-in keychain bridge past any host vetting only the typed field. Built-ins
are now gated by `allowedCapabilities` alone, the key namespace resolves custom
providers only, and a key that spells a built-in is rejected on the provider path
as defense in depth. Granting a built-in by its `CapabilityID` is unchanged.
Review findings P0 #1 and #2.
`Int.init(_: Double)` traps on non-finite and out-of-range input, and every
number reaching these paths is attacker-controlled: `JSONValue.intValue` backs 48
`.int(...)` call sites, and `JavaScriptExecutionRequest.decodeTimeoutMs` runs on
model-authored tool JSON before any script executes. `{"timeoutMs": 1e300}` or
`fetch(url, { timeoutMs: 1e300 })` killed the host process in-process.
- `JSONValue.intValue` goes through a total `exactInt(from:)` that rejects NaN,
infinity, and out-of-range magnitudes; in-range values truncate toward zero as
before.
- `decodeTimeoutMs` rejects doubles and numeric strings it cannot represent.
- `timeoutMs` is clamped to the 1...60000 bounds the tool schema already
advertises, in both the memberwise init and the decoder, so an over-large
budget yields the ceiling rather than an unbounded run.
- A declared-`.number` argument must now be finite: the type check rejects
non-finite JSON numbers, and `coerceScalarString` no longer accepts
`"inf"`/`"nan"`, which `Double.init(String)` parses happily.
Review finding P0 #3.
Both operations unconditionally recursive-deleted any existing destination, with
no overwrite flag and no directory check, and containment alone admits a root:
`DefaultPathPolicy.resolve(path: "documents:")` yields the documents root and
`isAllowed` accepts it via `path == allowed`. So
`apple.fs.move({ from: 'tmp:junk.txt', to: 'documents:' })` removed the user's
entire Documents directory. `fs.delete` already gated directories correctly, so
the three operations disagreed.
Destinations now go through one guard shared by move and copy: a sandbox root is
refused outright, an existing destination requires `overwrite: true`, and an
existing *directory* additionally requires `recursive: true` — the same shape
`fs.delete` enforces. `fs.delete` picks up the root check too, since
`{path: 'documents:', recursive: true}` had the same reach.
Also bounds and clarifies file IO:
- `CodeModeConfiguration.fileSystemLimits` caps `fs.read`/`fs.write` (16 MB
default). A 2 GB file in `documents:` was previously read whole and
base64-serialized. The read cap is checked against stat before the read, so it
does not spend the memory it exists to protect; the write cap is checked before
the parent directory is created, so a refused write leaves nothing behind.
- A `utf8` read of undecodable bytes now fails with a repair suggestion instead
of silently returning `""`, which told the script the file was empty.
- `encoding` is a `CodeModeStringEnum`, so the catalog advertises
`allowedStringValues` instead of leaving the constraint in prose.
`PathPolicy` gains `allowedRoots` (defaulted empty, so custom policies still
compile) to make "is this a root" answerable.
Golden metadata baseline regenerated; the diff is the four fs entries above.
Review finding P0 #4, plus the `fs.read` encoding-constraint item from P2.
The shipping default was `URLSession.shared`, which reads `HTTPCookieStorage.shared` and `URLCredentialStorage.shared`. Every script request therefore rode whatever the host app was already logged in to — authenticated session-riding against any origin with a live cookie — and a `Set-Cookie` in a script-fetched response poisoned the app's cookie jar. The tests all passed an `.ephemeral` session, so the shipping default was the one configuration never exercised. `network.fetch` now defaults to `NetworkBridge.isolatedSession`: ephemeral, with no cookie storage, no credential storage, no shared cache, and cookies disabled outright. Per-request cookie handling is disabled too, so a host that supplies its own session still does not attach its jar. Caller-supplied `Cookie`, `Authorization`, and `Proxy-*` headers were copied to the wire verbatim; they are now refused with `NETWORK_POLICY_VIOLATION` and audited. `NetworkAccessPolicy.allowsCredentialHeaders` re-enables them for hosts whose scripts legitimately call an authenticated API, and `.permissive` sets it, keeping that opt-out a faithful "old behavior" switch. Review finding P0 #5.
Nothing capped output. The whole result was stringified, copied into a Swift String, and re-decoded; every console.log was retained and then copied again into each CodeModeToolError; the events AsyncStream used the default unbounded buffering, so a chatty script whose events the host was not draining buffered without limit. One script could jetsam an iOS host. `CodeModeConfiguration.executionLimits` bounds all of it, and every limit degrades rather than fails — for an LLM consumer a truncated result with a diagnostic beats an out-of-memory kill: - Result size is measured *inside* JS, so an oversized value is never brought across. Over the limit, the output is replaced by a still-parseable descriptor (`__codeModeTruncated`, character count, limit, preview) plus a RESULT_TRUNCATED diagnostic suggesting fs.write + return the path. - Retained logs are capped at the first N with a one-time LOG_LIMIT_REACHED notice; streaming stays live past the cap since it is the retained copy that costs. Over-long messages are elided in the middle, keeping head and tail. - Diagnostics and permission events are capped. - The events stream uses `.bufferingNewest(n)`. Two watchdog findings in the same code path: - The serialization re-arm used `max(timeoutMs, 1000)`, so a 60s execution bought another 60s for JSON.stringify. It is `min` against a fixed 1s budget now, which is what "bounded budget" meant. - `watchdog.termination` was never consulted after the re-arm, so a runaway getter surfaced as `INVALID_RESULT: must be JSON-serializable` and steered the model to the wrong fix. It now reports EXECUTION_TIMEOUT/CANCELLED naming the serialization phase. Review finding P0 #6 and the first two P2 items.
**Location.** `CLLocationManager` delivers delegate callbacks on the run loop of the thread that *created* it, and the bridge's GCD workers have none. The manager and delegate were constructed on the execution thread with only `requestWhenInUseAuthorization()` dispatched to main, so `locationManagerDidChangeAuthorization` never arrived: every first-time grant burned the full 10s wait and then reported from a status re-read that races the TCC write, so a user who tapped Allow could still get `notDetermined`. Creation, delegate assignment, and the request now all happen on main, the delegate holds the manager alive (CoreLocation does not retain its delegate, and the manager would otherwise die with the dispatched block), and the resolved status comes from the callback rather than a re-read. **Reminders.** `EventKitBridge.readReminders` wrote `var result` from EventKit's completion queue, read it after a `semaphore.wait` whose timeout result was discarded, and returned `.array([])` on timeout — a data race with the late callback, and a result the script could not tell apart from "no reminders". Both now go through one `CompletionWait` helper that either returns the value the callback delivered or throws `BridgeError.timeout`, keeping the box alive so a late callback writes somewhere harmless. The broker's `request*` methods are migrated to the same helper, which also collapses their duplicated availability-branch semaphore plumbing. Review findings P0 #7 and #8.
**One ISO 8601 parser.** Health and Alarm fell back to `.withFractionalSeconds`;
EventKit, Notifications, and the system-UI validators used a bare
`ISO8601DateFormatter`. So `2026-07-24T10:00:00.000Z` — what JavaScript's
`toISOString()` emits, and what models therefore produce by default — parsed in
some bridges and not others. Worse, `calendar.read`/`reminders.read` then did
`isoDate(...) ?? Date()`, handing the script a plausible but wrong window with no
diagnostic. `CodeModeDate` is now the only parser, and a present-but-unparseable
date is an error; only an absent one takes the default.
**HealthKit stops claiming access it cannot verify.** `requestAuthorization`'s
`success` means the sheet flow completed — HealthKit deliberately hides read
denial so an app cannot infer a condition from it — but the bridge reported
`granted: true`, and mapped `getRequestStatusForAuthorization == .unnecessary`
("nothing left to ask") to `.granted`. Reads then returned [] while the
transcript claimed permission. It now reports flow completion, per-type *share*
authorization (the only part HealthKit answers), and says plainly that read
authorization is undisclosed. The broker's fail-closed `healthKitStatus` is
untouched.
**HomeKit status no longer prompts.** Constructing `HMHomeManager` is what
triggers the TCC dialog, and `homeKitStatus()` built one inside `status(for:)`
while `requestHomeKitPermission` called it twice and built a third. Status now
answers from a shared manager if one exists and otherwise fails closed with
`.notDetermined`, routing the caller to the request path where a prompt belongs.
**Network and browser destinations.** Bare single-label hosts (`router`,
`intranet`, `wpad`, `nas`) resolve onto the LAN through the DHCP search domain
but fell through to "allowed"; they are refused like `*.local`, and an explicit
`allowedHosts` entry still wins. `web.ui.present` and `auth.ui.webAuthenticate`
now consult `NetworkAccessPolicy` — opening a page in a browser was exempt from
the policy governing fetch — and `auth.ui.webAuthenticate` defaults to an
ephemeral browser session, so a script-started OAuth flow no longer begins
already signed in as the user.
**Crash and containment fixes.** `EKEvent.calendar` (`EKCalendar!`, nil for
orphaned events) and `EKReminder.title` (`String!`) are nil-coalesced like every
neighbouring field. Two force-unwrapped `URL(string:)!` in the public
`UIKitSystemUIPresenter` throw instead. A symlink to a *nonexistent* out-of-root
target passed containment, because `fileExists` follows links (so the link read
as a missing component) and `resolvingSymlinksInPath` leaves a dangling link
unchanged; both halves are fixed, with a bounded walk for cycles.
**Concurrency honesty.** `Task.isCancelled` was checked from plain GCD workers
where it is always false — removed, leaving the controller that `cancel()`
actually sets. The watchdog deadline is a `ContinuousClock.Instant`, so an NTP
adjustment cannot move it. `JavaScriptExecutionCall` cancels on deinit, so a
dropped call stops instead of running to completion in the background, and
`result` no longer spawns a `Task.detached` per access.
**Diagnostics.** Error-severity diagnostics were recorded but never emitted, so a
host watching the stream saw every diagnostic except the ones that mattered most.
**Path arguments.** `speech.file.transcribe` and `passkit.pass.add` left the raw
script-supplied `path` in the dictionary alongside `resolvedPath`; the raw value
is now replaced, so a host adapter reading `path` cannot bypass the path policy.
Golden baseline regenerated for the two web-auth metadata strings.
`EvalRunner` overwrote `executionOutput`/`Logs`/`Diagnostics`/`observedError` on every step and did not stop on an intermediate error, so validation only ever saw the last one: a scenario could pass while step 1 failed in a way step 2 masked, and step 1's logs — the evidence for why — were discarded before anyone could read them. Logs and diagnostics now accumulate across steps. The graded output and error are still the last step's, so a successful repair step clears the failure that preceded it, but an unexpected failure stops the run. "Unexpected" needs to be explicit, because repair scenarios deliberately call a helper wrong and fix it from the structured error — that first failure is the whole point. `CodeModeEvalExecuteStep.expectsFailure` marks those, and `filesystemRepairAfterInvalidArguments` moves to explicit steps to use it. Every other step failing now ends the scenario.
REVIEW.md P1 #12 asks the CodeMode target to drop its CodeModeMacros dependency by checking in the 117 expanded tool definitions. Declined, with reasoning in TODO.md: the cited cost is already measured and mitigated (prebuilt swift-syntax is default-on, and the version range is already widened for the one named conflict), the migration is one-way and strips @ToolParam hints off the property declarations, and it pulls against findings #11 and #13, which both want more generation from this metadata.
Cloudflare's central Code Mode insight is that models write markedly better code
against real types, and nothing here emitted any: the model got a flat
`argumentTypes` map of six coarse cases, a prose `resultSummary`, and constraints
that lived only in hint text it had to notice and obey.
The registry already holds names, required/optional arguments, types, enum
constraints, and hints, so `TypeScriptDeclarations` emits `.d.ts` from exactly
that. Constrained arguments become string-literal unions — the constraint *is*
the type. Dotted argument paths (`options.timeoutMs`) rebuild into nested object
types. Hints become doc comments; capability ID, result summary, aliases, and the
example ride along.
Two entry points:
- `JavaScriptAPIReference.dts` — per capability, so code-driven search stays the
filter and TypeScript becomes the payload. The tool description now points the
model at it.
- `CodeModeAgentTools.typeDeclarations()` — the whole platform-filtered surface,
namespaced, for a host system prompt. `capabilities()` is exposed alongside it,
which hosts also needed for consent UI and for building a `CapabilityGrant`.
Results are typed `CodeModeValue`: built-in bridges return untyped JSON
dictionaries, so a narrower result type would be a fiction. Per-capability result
schemas can tighten this later without changing the shape.
Generating declarations surfaced real catalog-vs-binding drift, which is fixed
rather than described (partial #11 — the hand-written bootstrap stays for now):
- `apple.keychain.{get,set,delete}` advertised object arguments while the wrapper
took positional strings, and the catalog's own example showed the positional
form. Code written from the metadata sent "[object Object]" as the key. The
wrappers accept both spellings; examples move to the object form.
- `fs.promises.rename`/`copyFile` could not pass the `overwrite`/`recursive` flags
that `fs.move`/`fs.copy` now require, so those aliases failed on any existing
destination.
- `apple.location.getCurrentPosition`/`getPermissionStatus` discarded a caller's
arguments outright; they now default the mode instead of forcing it.
- The tool description claimed "apple.* helpers take one object argument" as a
blanket rule; it now scopes positional to `fs.promises.*` and points at `dts`.
The Node-compatibility globals stay in a hand-authored preamble, because their
positional convention is not something the catalog can express — that gap is
exactly what #11 exists to close.
Partial P1 #9. The full async bridge ABI — Swift-resolved promises, concurrent native calls, bounded execution slots — remains a milestone; these are the parts that are wrong *within* the synchronous model, including both cheap wins the review called out as available immediately. **setTimeout was a lie.** It invoked the callback synchronously and ignored the delay, so three things were wrong at once: `clearTimeout` could never cancel, an error thrown by a callback propagated to setTimeout's *caller* rather than being an uncaught task error, and `await new Promise(r => setTimeout(r, 2000))` — the standard backoff — became a hot loop hammering remote APIs through `fetch`. There is now a real timer queue drained by the host between JavaScript turns: callbacks are deferred, delays are honoured, registration order breaks ties, `clearTimeout` works, and a throwing callback becomes a `TIMER_CALLBACK_ERROR` diagnostic. Waiting stays responsive to `cancel()` and bounded by `timeoutMs`, so a 30s timer under a 100ms budget still times out at 100ms. **Unsettleable promises are diagnosed, not waited out.** `waitForSettlement` polled a state that, under this model, only a timer can change — microtasks have already drained inside `evaluateScript` and no native call is outstanding. With no timer queued the promise is *provably* unsettleable, so it reports `JS_RUNTIME_ERROR: ... can never settle` immediately instead of sleeping a thread to the deadline for a result that cannot arrive. **Swallowed bridge failures are visible.** Nothing installs an unhandled-rejection hook, so a forgotten `await` — the most common LLM JavaScript mistake — discarded a `CAPABILITY_DENIED` and the agent was told the run succeeded. A script that completes successfully despite failed bridge calls now carries a `BRIDGE_FAILURES_NOT_SURFACED` warning naming them. It is a warning, not an error, because a deliberate try/catch is indistinguishable from a missing await. Tool description and README updated: setTimeout is no longer described as synchronous, `Promise.all` is documented as sequential (which it is, until the async ABI lands), and the new diagnostics have repair guidance. The eval suite splits the old "unresolved promises time out" scenario into an unsettleable-promise case and a genuine CPU-bound timeout, and gains a timer-backoff scenario.
The execution queue is `.concurrent` with no width limit, and each execution blocks its thread for the whole run, so thirty parallel `executeJavaScript` calls meant thirty blocked GCD threads and thirty live JavaScriptCore VMs. There was also no concurrent-execution test at all, despite the concurrent queue being a core design point. `ExecutionLimits.maxConcurrentExecutions` (default 8) gates the queue with a semaphore acquired on the worker rather than before dispatch, so the caller's async context is never blocked and waiting for a slot does not eat the per-execution `timeoutMs`. Two tests: twelve executions through three slots all complete with correct, isolated results and no state carried between contexts; and executions beyond the limit queue rather than fail. Closes the remaining "bound on concurrent executions" item in TODO.md §1.
Partial P1 #13. The provider-per-method macro and the MCP adapter remain; this
removes the blocker they both sit behind and closes the metadata gaps.
**The catalog was frozen at init.** `BridgeCatalog` snapshotted the registry in
`CodeModeAgentTools.init`, so `CapabilityRegistry`'s public post-init
`register(...)` methods were unreachable in the supported flow — and if reached,
would have desynced what search advertises from what execution can invoke. The
registry now carries a generation counter and the catalog rebuilds when it
changes, so the two can no longer disagree. `CodeModeAgentTools.register(provider:)`
exposes this: a host whose domain APIs depend on runtime state — a signed-in
account, a loaded document — can register them when that state arrives, and
search sees them immediately.
**Host providers can express constrained values.** `@CodeMode` rejected any
non-primitive property type, so a host's `allowedStringValues` was always empty
while built-ins advertised theirs — the model had no way to learn a provider's
accepted spellings. An unrecognized simple type is now taken to be a
`CodeModeStringEnum`, exactly as `@BuiltInCodeMode` already assumed, and its
accepted values reach the catalog, the generated TypeScript union, and argument
validation. A denylist of common non-enum types (`Date`, `URL`, `UUID`, …) keeps
the "unsupported property type" diagnostic pointing at the property rather than
letting a natural mistake fail inside generated code.
**Generated metadata is no longer vacuous.** `example` was always
`await path({})` regardless of arguments; it is now a call the model can
pattern-match, with a placeholder per required argument. `tags` was only the
parent path, so `myapp.tasks.complete` was findable only by searching
"myapp.tasks"; every path segment is now a tag.
EventKit's serialization logic executed nowhere — not in unit tests, not in CI, not in evals — because the bridge hard-instantiates `EKEventStore` and the tests can only reach permission denial. The two implicitly-unwrapped fields that crashed the execution thread are now covered directly by constructing the model objects, which needs no permission: an `EKEvent` with no calendar and an `EKReminder` with no title. CI runners are pinned to `macos-15` rather than `macos-latest`, so the package's declared macOS 15 floor cannot drift without a code change. TODO.md gains a "review follow-up — still open" section listing every part of REVIEW.md's P1 items that was not completed — the async handler signature and concurrent Promise.all, native-call interruption, generating the JS bootstrap, `@CodeModeProvider`/`@CodeModeTool`, the MCP adapter, the remaining bridge store seams, a simulator job, and the "prompt pending" permission status — so the finished parts are not mistaken for the whole. It also records that the review's claim about missing `FetchTaskHandler` redirect and size-cap coverage was stale; both are covered in NetworkAccessPolicyTests.
Quality pass over this branch's changes. No behavior change except one bug the refactor exposed, noted below. Reuse: - One `BridgeInvocationContext.toolError(...)` replaces three separate places assembling `CodeModeToolError` from the same three transcript accessors — the runtime's `toolError`, `SettlementParameters.error`, and `terminationError`. It belongs on the context, which owns those fields. - One `stringArray(from:evaluating:)` replaces the duplicated `JSON.stringify` → decode `[String]` round-trip in `rejectedSuggestions` and the timer-error read. - One `FileSystemBridge.overLimit(...)` replaces three hand-built size-limit messages that had already drifted apart in wording. Simplification: - `SettlementParameters` bundles the six values `waitForSettlement` and `runDueTimers` both needed; the two signatures drop from 7 and 8 parameters to 2 and 3. `checkInterrupted` replaces the cancellation/termination/deadline ladder both were repeating. - The host-withheld denial branch is hoisted above the `switch` instead of being nested identically inside two cases. - `decodeOutput` checks the watchdog once rather than side by side in a guard's else and again after it; its embedded JS returns the in-range value from one place instead of two. - `ExecutionTranscript.record(log:)` now matches the shape of `record(diagnostic:)` — one lock/unlock, no branch-local unlocking — and drops `droppedLogs`, which was incremented and never read. - `TypeScriptDeclarations.functionSignature` drops an unused `name:` parameter, and `lastComponent(of:)`, which existed only to supply it. - `CapabilityGrant.Resolution` keeps `withheld: [String]` instead of two sets that no caller read except through the accessor that joined them. - Dropped `CodeModeDate.string(from:)` — never called. Efficiency: - `advanceTimers` returns its callbacks' uncaught errors instead of buffering them for a second `takeTimerErrors` call, halving the bridge round-trips per timer tick. - The elapsed-time computation reads the clock once through a `Duration` extension rather than twice through a two-term attosecond conversion, and drops a `max` that could not fire. Bug found while refactoring: bundling the interrupt checks moved the deadline check ahead of the settled-state read, which is exactly the spurious-timeout regression the original comment warned about — a script that fulfilled a hair past the deadline would have had its result discarded. Ordering restored (cancellation before the read, deadline after) and pinned by a new test, which nothing covered before.
The tool descriptions and the generated TypeScript answer "what is this helper called and what does it take". Nothing answered "what shape should my work take", so an agent could reasonably use executeJavaScript as a thin RPC — one call per operation — which is exactly the pattern this package exists to replace. Every code example in the package was a *search* example; executeJavaScript had none. `CodeModeAgentGuidance` supplies the missing half, to pair with `typeDeclarations()`: one call runs the whole task, filter and aggregate in the script, return the graded answer rather than the raw data, request only the capabilities you call. Three tiers with an optional budget selector, since prompt space is the scarce resource: - `.brief` (~160 tokens) — the core idea alone - `.standard` (~580) — adds the search → write → repair workflow and a worked multi-step example - `.full` (~1000) — adds partial-failure handling, the sequential-native-call reality so Promise.all is not over-promised, and the anti-pattern list `systemPrompt(approximateTokenBudget:)` picks the largest tier that fits. Tiers are additive — a smaller one loses examples, never a rule, which a test pins by substring containment. Token counts are estimated at four characters per token and documented as a planning bound, not a real tokenization. A budget too small for any tier still returns `.brief` rather than nothing. Also in executeJavaScript's description: the paradigm now leads instead of the namespace tour, three worked examples replace having none, and capability minimization is stated plainly — the eval suite asserts on it 38 times but the description mentioned it once in passing, so we were grading an unstated rule. Guarded, because documentation that names a helper the runtime lacks teaches the exact failure it is trying to prevent: tests check every helper and argument in both the guidance and the tool description against the live catalog, and run the worked example end to end. That caught a wrong example on the way in — `contacts.read` takes `identifiers` (a plural array), so the per-item loop it showed was the anti-pattern, not the pattern; it is now the example for "prefer one call that takes a collection". The new `fs.whole-job-one-script` eval scenario grades the standard rather than just asserting it: unit tests confirm a transcript splitting one job across three executions fails on tool order, an over-broad capability list fails even with the right answer, and the single-script transcript passes.
Four failures on PR #7, all reproducible locally with the exact CI commands. **iOS build — a real break in my own CoreLocation fix.** `LocationPermissionDelegate` crosses an isolation boundary: the manager is created inside a `DispatchQueue.main.async` block, which is main-actor-isolated on iOS, while `wait`/`observedStatus` are called from the requesting thread. Swift 6 rejects sending a non-Sendable delegate across that. It is genuinely thread-safe — all mutable state is lock-guarded, the handoff is a semaphore — so it is now `@unchecked Sendable` with the reasoning recorded. `swift test` on macOS never caught this because `requestLocationPermission` is inside `#if canImport(CoreLocation) && os(iOS)`, so the macOS build never compiled the branch. Exactly the gap TODO.md flags about iOS being build-only. **visionOS build — latent, surfaced by pinning the runner.** `openSystemURL` calls main-actor-isolated `UIApplication.shared.open` from a nonisolated synchronous context. This is pre-existing code I did not touch; pinning `macos-latest` to `macos-15` moved the toolchain to one that diagnoses it. Fixed with `MainActor.assumeIsolated` on the branch that has already established it is on the main thread — a hop would return before the open was requested. **Three load-sensitive test bounds.** Individual tests took 8-9s on the CI runner against ~0.1s locally, so wall-clock upper bounds written on an idle machine failed. Each of these is a hang-guard, not a performance assertion, and the thresholds now say so: - `setTimeoutActuallyWaitsTheRequestedDelay` keeps its lower bound, which is the actual claim (the delay was previously ignored entirely); the upper bound is dropped since the script's own 5s timeoutMs already fails a runaway wait. - The remaining bounds are loosened to stay well under the thing they prove did not happen — 20s against a 30s budget, 15s against a 20s timer. - `completionWaitToleratesALateCallback` waits 60s. The starvation is real: parallel tests plus this runtime's thread-blocking timer waits can leave a 0.2s dispatch waiting seconds for a GCD worker. That is a property of the synchronous bridge model, and one more argument for the async ABI in #9. Also fixes the `variable 'root' was never mutated` warning CI surfaced in TypeScriptDeclarations. Verified locally with the exact CI invocations: `swift test` (322 passing) plus `xcodebuild -destination generic/platform={iOS,visionOS}`, both succeeding.
CI caught a real bug in the timer queue, not just a flake. `advanceTimers` sorted due timers by registration id alone. That is correct only when everything came due at the same instant. When the host's sleep overshoots far enough that two timers with *different* delays land in one tick — which is routine on a loaded machine — the later one fired first purely because it was registered first. Timers now sort by due time, using registration order only to break ties, which is what a real event loop does. The existing test could not catch this: with delays of 5ms and 10ms it only fails when the sleep overshoots, so it passed locally and failed on CI. It is joined by one that reproduces the condition deterministically — delays below the host's sleep granularity, so both timers reliably come due in a single call — plus one covering the same-instant tie-break the id sort did get right. Verified by reverting the fix: the new test fails, the old one still passes. Separately, `completionWaitReturnsTheDeliveredValue` timed out waiting 5s for an immediate `DispatchQueue.global().async`. That is thread-pool starvation: the suite runs in parallel and this runtime's timer waits block GCD workers with `Thread.sleep`, so a trivial dispatch can wait seconds for a thread. The timeout is now 60s, since the assertion is that the value arrives rather than how fast. Worth recording that the starvation is a property of the synchronous bridge model — the async ABI in #9 is what actually removes it.
A second read of the branch as a reviewer would, rather than as its author.
Two of the findings are correctness bugs in code this branch added.
**Cancel-on-deinit cut off hosts that only iterate events.** The P2 fix made
`JavaScriptExecutionCall` cancel in its `deinit` so a dropped call stops running.
But Swift does not keep a local alive to the end of its scope: a host that reads
`call.events` and never touches `call` again can have it released mid-iteration
in an optimized build, and the deinit then cancelled the execution out from under
the `for await`. The README Quick Start avoids this only because it awaits
`call.result` afterwards. The right semantic is "cancel when nothing can observe
the outcome" — so an `ExecutionObservationToken` is now held by both the call
and the stream's continuation storage, and the execution runs while *either* is
reachable. Two tests discriminate by construction: iterating events after
explicitly releasing the call must see `finished`, and dropping both must free a
single-slot runtime for the next call.
**The concurrency bound blocked the threads it was protecting.** The slot
semaphore was acquired inside `executionQueue.async`, so every execution beyond
the limit held a GCD worker while waiting — exactly the resource the limit exists
to conserve. `ExecutionSlots` is an actor; waiters park as suspended tasks, a
freed slot hands directly to the next waiter, and a call cancelled while parked
throws before anything is dispatched. Tested: a call cancelled while waiting
reports CANCELLED and never ran (the file it would have written is absent).
**The timer loop polled.** `runDueTimers` slept in 5ms slices — 200 wakeups a
second per sleeping execution, and up to 5ms of cancellation latency — which is
what made CI's thread starvation visible. `ExecutionCancellationController` now
owns an `NSCondition`: one interruptible wait until the timer is due or the
deadline passes, woken immediately by `cancel()`. The wall-clock `Date` is only
the wake-up hint; the loop still re-checks against `ContinuousClock`, so an NTP
adjustment cannot move either the timer or the deadline.
**Smaller findings:**
- The per-capability `dts` flattened `apple.fs.read` to `declare function
apple_fs_read`, a name that does not exist. It is now declared at its real call
path; bare globals like `fetch` are declared directly.
- `health.permission.request` changed its result shape in the P2 commit but its
advertised `resultSummary` still promised a `granted` field. Golden regenerated.
- `HostConfigurationValidator.validate(configuration:)` warns when
`capabilityGrant` is `.unrestricted`, and the README Quick Start — the thing
that gets copy-pasted — now models a grant instead of the unenforced default.
- `hostWithheldCapabilityIdentifiers` was public API for internal bookkeeping.
- `ExecutionLimits` is documented as bounding concurrency too, not just "a single
execution", since `maxConcurrentExecutions` lives there.
- `CodeModeAgentGuidance.Length` drops `Comparable` and a private ordering it
carried only so the budget selector could sort; `allCases` already is the order.
Verified with the exact CI invocations: `swift test` (331 passing) and
`xcodebuild -destination generic/platform={iOS,visionOS}`.
The test cancelled 50ms after *dispatching* the sleeper block. On a starved CI runner the dispatch alone can take seconds, so the cancel arrived before there was anything to wake, the sleeper only ran after the test had given up, and the failure read as "cancel did not wake the sleep" when the sleep had never begun. It now waits (generously) for the block to signal that it is about to sleep, then cancels and measures only the wake-up — which is the property under test. If the cancel lands in the window before the wait starts, `sleep(until:)` sees the flag on its first check and returns immediately, so there is no hang and no false positive.
The last CI run failed four tests at ~66 seconds each — including two that need nothing but a single GCD thread. That is a runner-wide stall, and the suite was partly causing it: seven watchdog tests each spin a core at 100% for their budget, and a dozen event-loop tests each hold a GCD worker for the length of a timer, all dispatched in parallel onto a three-core runner. Serializing those three suites keeps them from stacking on each other; cross-suite parallelism is unchanged. Two tests were also asserting more than their property, which is why a stall read as a runtime bug: - `executeCancelsWhenAwaitingTaskIsCancelled` cancelled after a `Task.yield()`. Under a stall the test task can be descheduled past the script's 20s timer, and cancelling a finished execution is a no-op — reported here as the script "ignoring" cancellation. It now cancels only after observing the script's own `running` log on the events stream, which makes the outcome CANCELLED regardless of scheduling. - `anUnsettleablePromiseIsReportedImmediately` and `aTimerStillPendingAtTheDeadlineTimesOut` carried wall-clock bounds that their error codes already proved: JS_RUNTIME_ERROR rather than EXECUTION_TIMEOUT shows the 30s budget was not run out, and EXECUTION_TIMEOUT rather than a returned `1` shows the 30s timer was not waited out. Dropped. No product code changed. Run locally: 331 passing.
`CodeModeAuthoringTests` uses `CodeModeConfiguration` and `CodeModeAgentTools`
but declared only `CodeModeAuthoring` and `CodeModeMacros`, reaching `CodeMode`'s
symbols through re-export. SwiftPM did not treat that as an edge for rebuild
invalidation, so whenever a `CodeMode` type changed layout the authoring test
objects went stale — and a plain `swift test` then either failed to link
("Undefined symbols: CodeModeConfiguration.init(...)") or crashed with signal 11
partway through the run. Every such failure this branch hit was worked around by
touching the test sources; this is the fix.
Verified by reproducing the trigger: add a stored property to
`CodeModeConfiguration`, run `swift test` with no workaround. Before: link
failure or segfault. After: the authoring tests rebuild and all 331 pass.
The timer tests were the least reliable tests in the suite, and the earlier
fixes — serializing suites, widening bounds — reduced the failure rate without
removing the cause: they slept real time and asserted wall-clock bounds, so a
loaded CI runner turned a 150ms wait into seconds and every assertion became a
guess about how slow the machine was allowed to be.
`RuntimeClock` is now the runtime's source of time for the two places that wait:
the timer loop and the watchdog deadline. `RealClock` is production and
unchanged. `VirtualClock` makes `sleep(until:)` a jump, so under test a timer
fires instantly and exactly, and a test can assert on how long the runtime
*asked* to wait rather than on how long the machine took:
- `setTimeoutActuallyWaitsTheRequestedDelay` asserts `clock.elapsed ==
.milliseconds(150)` — exact, where it previously asserted `>= 0.14` real
seconds with no upper bound it could safely hold.
- `aTimerStillPendingAtTheDeadlineTimesOut` asserts it stopped at exactly 100ms,
never anywhere near the 30s timer.
- `anUnsettleablePromiseIsReportedImmediately` asserts `elapsed == .zero` — the
runtime never slept at all.
- `aResultThatSettlesRightAtTheDeadlineIsNotDiscarded` was a 12-iteration race
hoping to land a timer on the deadline; it is now one exact case with timer
and deadline at the same instant.
`VirtualClock(overshoot:)` models a slow machine precisely — every sleep wakes
late by a fixed amount. That matters for `timersComingDueInOneTickStillFireInDueOrder`:
after the earlier switch from 5ms polling to an interruptible wait, real sleeps
stopped overshooting enough to bring two timers due in one tick, so that test
had quietly stopped exercising the condition it exists for. With a 10ms overshoot
it reproduces it on every run.
Both converted regression tests were mutation-checked: the overshoot test fails
against the id-only sort, and the exact-deadline test fails against a
deadline-before-settled-state check — the two bugs they guard.
Kept on the real clock deliberately: watchdog termination of a CPU-bound script
(driven by JavaScriptCore's own execution-time callback, which virtual time cannot
advance) and the cancel-mid-sleep tests, which are already outcome-deterministic
because they synchronize on observable events rather than durations.
The seam is internal — `CodeModeAgentTools.init(config:clock:)` is not public.
Production time is not a host decision. Verified with the exact CI invocations:
`swift test` (331 passing) and `xcodebuild -destination generic/platform={iOS,visionOS}`.
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
1 participant
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.
Works through the findings in
REVIEW.mdonorigin/claude/codemode-swift-review-8zcpo2. All 8 P0 and all P2 items are done; P1 is partial — what was left, and why, is recorded inTODO.mdunder "REVIEW FOLLOW-UP — STILL OPEN" so the finished parts aren't mistaken for the whole.322 tests passing, up from 270. One commit per finding group, each with the reasoning in its message.
P0 — all eight
CapabilityGrant—allowedCapabilitiesis a model-authored tool argument and nothing narrowed it, so a host piping tool JSON straight intoexecuteJavaScript(the README Quick Start shape) granted whatever the script asked itself for. The host's grant is now intersected with the request. Also closes the aliasing hole:allowedCapabilityKeysaccepts arbitrary strings and isn't validated againstCapabilityID, butinvokeBuiltInhonoured it — so{"allowedCapabilities": [], "allowedCapabilityKeys": ["keychain.read"]}reached the built-in keychain bridge past any host vetting only the typed field.Int(Double)traps on non-finite and out-of-range input, and every number on these paths is attacker-controlled.{"timeoutMs": 1e300}killed the host process before any script ran.fs.move/fs.copy— both unconditionally recursive-deleted an existing destination, and containment admits a root, sofs.move({ from: 'tmp:junk.txt', to: 'documents:' })removed the user's Documents directory. Now gated byoverwrite/recursivematchingfs.delete, which also picks up the root check.network.fetch— defaulted toURLSession.shared, so script traffic rode the app's cookies and stored credentials. Now an isolated ephemeral session; credential headers refused unless the host opts in. The tests all passed.ephemeral, so the shipping default was the one configuration never exercised.ExecutionLimits— nothing capped result, logs, diagnostics, or event buffering. Result size is now measured inside JS so an oversized value is never brought across, and every limit degrades with a diagnostic rather than failing.CLLocationManagerwas created on a run-loop-less GCD worker, so its delegate never fired and every first-time grant burned 10s then reported a raced status.readReminderswrote avarfrom EventKit's completion queue and returned[]on a discarded timeout. Both go through oneCompletionWaithelper.P2 — all of them, one commit
One ISO 8601 parser that rejects rather than substituting "now"; HealthKit no longer claiming access it can't verify; HomeKit status that doesn't trigger a TCC prompt; bare single-label hosts (
router,intranet) blocked; ephemeral web-auth sessions + network policy applied to browser-opened URLs; theEKEvent.calendar/EKReminder.titleimplicit-unwrap crashes; dangling-symlink containment;ContinuousClockdeadlines; error-severity diagnostics reaching the event stream; raw paths no longer forwarded alongside resolved ones.P1 — partial, deliberately
Done: TypeScript declarations generated from the registry (#10) — per-capability
dtsplus whole-surfacetypeDeclarations(). Generating them exposed four real catalog-vs-binding drifts, includingapple.keychain.get, where code written from the metadata sent"[object Object]"as the key.A real timer loop (part of #9) —
setTimeoutused to fire synchronously and ignore its delay, soclearTimeoutcould never cancel andawait new Promise(r => setTimeout(r, 2000))was a hot loop hammering remote APIs. Unsettleable promises now fail fast instead of running out the clock; swallowed bridge failures surface asBRIDGE_FAILURES_NOT_SURFACED; concurrent executions are bounded.The catalog is unfrozen (part of #13) so post-init
register(provider:)works and can't desync search from execution, and host providers can finally express enum constraints.Not done: the async handler signature and concurrent
Promise.all, native-call interruption, generating the JS bootstrap,@CodeModeProvider/@CodeModeTool, the MCP adapter, store seams for the five system-framework bridges, a simulator job.Declined: #12 (drop swift-syntax from the core target) —
PLAN-registration-macros.mdrecords this as a measured, owner-approved decision, prebuilts are default-on, and the version range is already widened for the one named conflict. Reasoning is inTODO.md.Also here
executeJavaScripthad no code examples at all.CodeModeAgentGuidanceadds.brief/.standard/.fulltiers (~160/580/1000 tokens) with a budget selector, plus worked examples in the tool description. Tests check every helper and argument in the docs against the live catalog — that caught one of my own examples using an argument that doesn't exist.BridgeInvocationContext.toolError, dead code removed, 7- and 8-parameter signatures bundled.For the reviewer
CapabilityGrant, theNetworkBridgesession swap, andPathPolicysymlink resolution. These change what a script can reach.JS_RUNTIME_ERRORinstead ofEXECUTION_TIMEOUT, andfs.move/fs.copynow fail on an existing destination withoutoverwrite: true.TODO.md:232flags that watchdog termination was only ever confirmed on macOS, and JSC ships per-OS. The new timer loop depends on that same watchdog, so the exposure grew. Worth doing before tagging0.1.0.Round 2 — fresh-look pass
Re-read as a reviewer rather than as the author. Two findings are correctness bugs in code this branch added, both now fixed and pinned:
call.eventswithout touchingcallagain could have the execution cancelled mid-loop in an optimized build. Now an observation token is held by both the call and the stream; cancellation happens only when neither is reachable.Also: the timer loop no longer polls in 5ms slices (interruptible
NSConditionwait, cancel wakes it immediately); per-capabilitydtsdeclares the real call path instead of a flattenedapple_fs_read;health.permission.request's advertised result summary matches its actual shape;HostConfigurationValidator.validate(configuration:)warns on an unrestricted grant; the README Quick Start models a grant.One more behaviour change for the reviewer list:
web.ui.presentandauth.ui.webAuthenticatenow consultNetworkAccessPolicy. A host that previously openedhttp://localhost:3000dev pages throughweb.ui.presentunder the default policy will needallowedHosts: ["localhost"]. This was in the P2 commit but not called out above.Round 3 — deterministic timer tests, and one manifest fix
The timer tests were the suite's least reliable, and the earlier mitigations (serialized suites, wider bounds) reduced the failure rate without removing the cause: they slept real time and asserted wall-clock bounds. Now:
RuntimeClockis injected into the two places the runtime waits — the timer loop and the watchdog deadline.RealClockis production, unchanged.VirtualClockmakes a sleep a jump, so timer tests are instant and assert exactly how long the runtime asked to wait (clock.elapsed == .milliseconds(150)), not how slow the machine was allowed to be. Internal seam;CodeModeAgentTools.init(config:clock:)is not public.VirtualClock(overshoot:)models a slow machine precisely. It matters: after the switch to an interruptible wait, real sleeps stopped overshooting enough fortimersComingDueInOneTickStillFireInDueOrderto hit its condition — it had quietly stopped testing the bug it guards. Both converted regression tests were mutation-checked against the bugs they exist for.Package.swift:CodeModeAuthoringTestsusesCodeModeConfiguration/CodeModeAgentToolsbut never declaredCodeModeas a dependency, reaching it via re-export. SwiftPM didn't invalidate those test objects whenCodeMode's layout changed, so a plainswift testafter a struct change failed to link or segfaulted. Reproduced, fixed, re-verified.