Address the codemode-swift-review findings: all P0 and P2, partial P1#7
Open
zac wants to merge 16 commits into
Open
Address the codemode-swift-review findings: all P0 and P2, partial P1#7zac wants to merge 16 commits into
zac wants to merge 16 commits into
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.
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.