Repository navigation
Conversation
contextablemark
left a comment
contextablemark
left a comment
There was a problem hiding this comment.
I’m comfortable removing kotlin-json-patch and reusing the existing implementation across all Kotlin targets. The stricter StateManager behavior is acceptable, including rejecting patches that previously succeeded through library leniency and comparing numeric test values correctly. There’s no need to retain the old library behind expect/actual or split the migration into another PR.
Before merging, please make these focused changes:
-
Make the shared patch applier validate its own input. It currently relies on callers to reject unknown operations and malformed pointers. Handle
addandreplaceexplicitly, reject unsupported operations, and validate bothpathandfrombefore decoding them. Reuse the existing validation rules where possible. Keep the pipeline’s unknown-operation filtering at its boundary. -
Check in the 65-case comparison as permanent common-test fixtures, with explicit expected outcomes for the behavior we’re adopting. Run these against the shared applier, and cover representative cases through both
StateManagerand the event pipeline, including atomic failure, error reporting, and recovery on a subsequent valid delta. Distinguish the application restriction against leaving state absent from malformed RFC patches. -
Add a macOS ARM64 test job to PR CI. Since this PR introduces that target, we should catch regressions before release staging.
To answer the remaining behavior and release questions explicitly:
- Unknown operations: preserve the current distinction for this PR—the pipeline skips unknown operations, while
StateManagerrejects the whole delta. Document and test that policy; aligning them would be a separate behavior change. - Exception types: the change is acceptable, but it should be called out alongside the stricter patch behavior.
- Version: the next Kotlin release should be 0.5.0, given the observable changes for existing users. The release workflow can manage the bump; this PR doesn’t need to do it manually.
- Release notes: no manual changelog entry is needed here; release preparation generates those entries. Please ensure the commit messages describe the
StateManagerbehavior and exception-type changes. We’ll review the generated notes when preparing the release.
The Kotlin 2.1.21 bump is also reasonable given the compiler/stdlib alignment you described.
Add macosArm64 and wasmJs (browser) targets to the core, client and tools modules. kotlin-json-patch publishes no macOS or Wasm artifacts, so drop it. The agent event pipeline already applied state deltas with its own RFC 6902 implementation and only borrowed the library's exception type; StateManager now uses that same implementation. - Share the Darwin HttpClient factory between iOS and macOS through an appleMain source set; wasmJs uses the Ktor Js engine. - Make CircuitBreakerTest use runTest so it runs on wasmJs. - Mark the six new klib-only publications as jar-less for JReleaser. - Run wasmJsBrowserTest in the Kotlin unit workflow.
StateManager now rejects deltas it used to accept (replace/remove of a missing member, add under a missing parent, move into its own child, add without a value), keeping the previous state, and a numeric test now compares by value (1 equals 1.0). For a rejected delta, onStateError now receives com.agui.client.state.JsonPatchApplicationException instead of the kotlin-json-patch exception types. kotlin-json-patch silently ignored those operations; the shared RFC 6902 implementation rejects the delta as a whole, as RFC 6902 requires. The applier validates its own input with the existing AgUiV1 patch rules before applying anything: only the six RFC 6902 ops, a well-formed path and from, and value/from where required. add and replace are handled explicitly and any other op is rejected. Invalid input and application failures are both reported as JsonPatchApplicationException. Unknown ops keep their current, different handling: the event pipeline drops them at its boundary and applies the rest, while StateManager rejects the whole delta. The behaviour is pinned by 73 common-test fixtures (RFC 6902 Appendix A plus edge cases) run against the applier, StateManager and the event pipeline, covering atomic failure, error reporting and recovery on the next valid delta. Leaving no state document is rejected as an application restriction, separately from malformed RFC patches and from RFC application errors.
publish.sh runs the Kotlin compiler in-process with a 384 MiB Metaspace cap. With the new targets that staging build failed twice over: - Wasm development links (used by wasmJsBrowserTest) enable debugger custom formatters and load custom-formatters.js through the system class loader. In-process, the compiler lives in Gradle's worker class loader, the resource is not found, and the link fails with a NullPointerException. Run KotlinJsIrLink tasks out of process. - allTests ran out of Metaspace. 512 MiB passed but Gradle still warned that Metaspace was nearly exhausted (as it already did at 384 MiB before this change); 768 MiB passes without the warning.
Ktor 3.2.4 already resolves kotlin-stdlib 2.1.21, and the Wasm compiler warns that a stdlib newer than the compiler (2.1.20) is unsupported. Use the matching compiler.
Add a macos-latest job to the Kotlin unit workflow that runs macosArm64Test for core, client and tools, so regressions in the new target show up before release staging. The client module declares an Android target, so the job installs the same Android SDK components as the release workflow. Move the test result summary into sdks/community/kotlin/scripts/summarize-test-results.sh so both jobs use it.
testAgentWithSingleTool and testAgentWithMultipleTools sent a message to https://test-api.com and ignored the result. On JVM the 30 s connect timeout fires inside runTest's budget, but the wasmJs browser runner (2 s) and macosArm64 (1 min) time out first, so both tests fail on the new targets. Drop the request; the tests only check the tool registry.
Python Preview PackagesVersion
Install with uvAdd the TestPyPI index to your [[tool.uv.index]]
name = "testpypi"
url = "https://test.pypi.org/simple/"
explicit = trueThen install the packages you need: # Core SDK
uv add 'ag-ui-protocol==0.0.0.dev1793194136' --index testpypi
# Integrations (each already depends on the matching ag-ui-protocol preview)
uv add 'ag-ui-langgraph==0.0.0.dev1793194136' --index testpypi
uv add 'ag-ui-crewai==0.0.0.dev1793194136' --index testpypi
# NOTE: ag-ui-agent-spec depends on pyagentspec (git-only, not on PyPI).
# You will need to install pyagentspec separately from its git repo.
uv add 'ag-ui-agent-spec==0.0.0.dev1793194136' --index testpypi
uv add 'ag_ui_adk==0.0.0.dev1793194136' --index testpypi
uv add 'ag_ui_strands==0.0.0.dev1793194136' --index testpypiInstall with pippip install \
--index-url https://test.pypi.org/simple/ \
--extra-index-url https://pypi.org/simple/ \
ag-ui-protocol==0.0.0.dev1793194136
Commit: 5fc872a |
@ag-ui/a2a-middleware
@ag-ui/a2ui-middleware
@ag-ui/event-throttle-middleware
@ag-ui/mcp-apps-middleware
@ag-ui/mcp-middleware
@ag-ui/a2a
@ag-ui/adk-js
@ag-ui/adk
@ag-ui/ag2
@ag-ui/agno
@ag-ui/antigravity
@ag-ui/aws-strands
@ag-ui/claude-agent-sdk
@ag-ui/claude-managed-agents
@ag-ui/crewai
@ag-ui/langchain
@ag-ui/langgraph
@ag-ui/llamaindex
@ag-ui/mastra
@ag-ui/pydantic-ai
@ag-ui/vercel-ai-sdk
@ag-ui/watsonx
@ag-ui/a2ui-toolkit
create-ag-ui-app
@ag-ui/client
@ag-ui/core
@ag-ui/encoder
@ag-ui/proto
commit: |
d4c71f1 to
bd17772
Compare
|
Thanks for the update—the three requested changes are now in place. One remaining concern: the Wasm test lockfile still sends all package downloads to Then run Once that's pushed, a maintainer can approve the new-head CI run and check both Kotlin jobs. I found no evidence of altered packages; the concern is reproducible builds without an unexplained mirror. |
The Wasm test lockfile pointed every resolved URL at a private npm mirror that came from the author's local npm configuration. Rewrite the host to registry.npmjs.org. Versions, sha1 fragments and integrity hashes are unchanged; all 301 entries match the npmjs registry metadata.
|
Thanks, Mark. I pushed LockfileThe mirror came from my local npm default registry. The commit only rewrites the host in the 301 To review it, a script fetched Test runCommit BUILD SUCCESSFUL in 4m 7s, with all 64 tasks executed. All 1522 tests passed with no failures or skips: No mirror contactI routed Gradle ( A proxy log can't show traffic that bypassed it, so I also checked the outputs. The fresh caches and Nothing was published. Both Kotlin jobs ( |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| # Built-in cache is fork-safe (fork PRs can't write to base repo cache) | ||
| - name: Setup Gradle | ||
| uses: gradle/gradle-build-action@ac2d340dc04d9e1113182899e983b5400c17cda1 # v3.5.0 |
| core/jvmTest client/jvmTest tools/jvmTest \ | ||
| core/wasmJsBrowserTest client/wasmJsBrowserTest tools/wasmJsBrowserTest | ||
|
|
||
| kotlin-macos: |
|
The remaining CI failure is This looks like a timing race in the test: its recovery window is only 20 ms. After the second failure, a pause before the assertion lets Could you make the breaker's clock injectable, defaulting to the system clock, and use a controllable clock in these tests? Keep time fixed while checking the failure threshold, then advance it explicitly past the recovery timeout for the half-open/recovery checks. That would remove the reliance on the 20/25 ms wall-clock waits. JVM passed 508 tests and macOS passed 507. After the fix, we still need the full Ubuntu JVM/Wasm job to pass; client Wasm tests hadn't run when Gradle stopped. |
Summary
Adds
macosArm64andwasmJs(browser) targets to the Kotlin SDK'score,clientandtoolsmodules.kotlin-json-patchpublishes no macOS or Wasm artifacts, so it is removed.StateManagernow uses the RFC 6902 implementation that the event pipeline (DefaultApplyEvents) already used.HttpClientfactory moves fromiosMainto a newappleMainsource set shared by iOS and macOS.wasmJsuses the KtorJsengine.CircuitBreakerTestusesrunTestinstead ofrunBlocking, which does not exist on wasmJs.kotlin-{core,client,tools}-{macosarm64,wasm-js}) are klib-only, so they get the samejar=falseoverrides as the iOS ones.exportReleaseCoordinatesnow lists 24 publications (18 before).kotlin-js-store/yarn.lockis the lockfile the Kotlin Gradle plugin generates for the Wasm test runner. Every entry resolves fromregistry.npmjs.org.Changes since the review on 10-13
applyStateDeltafirst runs the existingAgUiV1patch rules: only the six RFC 6902 ops, a well-formedpathandfrom, andvalue/fromwhere required. It then applies all six ops itself and rejects any other op. Every failure, both invalid input and application errors, is thrown ascom.agui.client.state.JsonPatchApplicationException.StateManagerno longer runs its own validation.StateDeltaFixtures.kt(commonTest) holds 73 cases with explicit expected outcomes: RFC 6902 Appendix A (except A.13, which needs duplicate JSON keys), plus edge cases for root documents, array indices, pointers, copy/move,testand malformed operations. They run against the applier,StateManagerand the event pipeline (the pipeline skips the unknown-op case, whichunknownOperationPolicycovers). The tests cover atomic failure, the exception type and delta passed toonStateError, and recovery on the next valid delta. The outcomes fall into four classes:Applied,Malformed(not a valid RFC 6902 patch),Unapplicable(an RFC 6902 application error) andAbsentDocument(RFC 6902 allows removing the root, but the application always keeps a state document). The test tells the classes apart by exception message; there is no exception subtype per class.I did not keep the harness behind the 65-case comparison in the original description, so this set was rebuilt rather than checked in as-is. It includes every row of the earlier comparison table.
unitworkflow has a newkotlin-macosjob (macos-latest) that runsmacosArm64Testfor all three modules. It installs the same Android SDK components as the release workflow, because the client module declares an Android target. The test summary moves tosdks/community/kotlin/scripts/summarize-test-results.sh, and both jobs use it.AgentToolIntegrationTest: two tests no longer send a request tohttps://test-api.com. Neither test used the result. They passed on JVM only because the 30 s connect timeout fires withinrunTest's budget, and they timed out on wasmJs and macosArm64.As you asked, the unknown-op policy stays as it is. The event pipeline drops unknown ops and applies the rest, while
StateManagerrejects the whole delta. This is documented inStateManagerand pinned byunknownOperationPolicy. There is no manual changelog entry. TheStateManagerbehaviour change and the exception-type change are described at the top of thefix(kotlin): validate state deltas in the shared patch appliercommit body.Behaviour changes for users
StateManagernow rejects deltas it used to accept:replace/removeof a missing member,addunder a missing parent,moveinto its own child,addwithout a value,addat index01, removing the root, and a patch whose later op fails. A rejected delta keeps the previous state and callsonStateError. A numerictestnow compares by value (1equals1.0).onStateErrornow receivesJsonPatchApplicationExceptionfor a rejected delta, instead of the kotlin-json-patch exception types. In the event pipeline, which deltas apply or fail is unchanged. Only the exception class changes, plus the wording of two messages: leaving no state document, and an operation after the root was removed.Not changed here:
StateManagerstill callsonStateDeltainside thetrythat reportsonStateError, and thatcatchalso catchesCancellationException. That behaviour predates this PR, and I'd send the fix as a separate PR.Testing
Local run on
077d257(Apple Silicon, JDK 21, fresh Gradle, Yarn and Kotlin/Native caches):./gradlew --no-build-cache --rerun-tasks jvmTest wasmJsBrowserTest macosArm64Testpassed all 1522 tests across core, client and tools (JVM 508, wasmJs 507, macosArm64 507).actionlintpasses on the workflow. Neither the updatedkotlinjob nor the newkotlin-macosjob has run on this head yet, because the PR comes from a fork. On the earlier headd4c71f1, thekotlinjob failed only on the twoAgentToolIntegrationTestcases thatbd17772fixes.Release staging (
publish.sh)Running
publish.sh --dry-runlocally exposed two failures that./gradlewwith the default settings does not show. Both come from the script's-Pkotlin.compiler.execution.strategy=in-processand-XX:MaxMetaspaceSize=384m:wasmJsBrowserTestuses), KGP turns on Wasm debugger custom formatters, and the compiler loadscustom-formatters.jsthroughClassLoader.getSystemClassLoader()(wasmCompiler.kt). In-process, the compiler runs in Gradle's worker class loader, the resource is not found, andFiles.copythrows. KGP has no switch to turn this off (KotlinJsIrLink), soKotlinJsIrLinktasks now run out of process. Other compilations stay in-process.allTeststhrewOutOfMemoryError: Metaspaceat 384 MiB. At 512 MiB it passes, but Gradle still warns that Metaspace is nearly exhausted.upstream/mainalready gets that warning at 384 MiB. At 768 MiB the warning goes away, and heap plus Metaspace stays well within amacos-latestrunner's memory.The commit
build(kotlin): bump Kotlin to 2.1.21bumps Kotlin from 2.1.20 to 2.1.21. Ktor 3.2.4 already resolves kotlin-stdlib 2.1.21, and the Wasm compiler warns that a stdlib newer than the compiler is unsupported. The bump is a separate commit, so it is easy to drop if you'd rather not take it here.