Upstream sync: post-v1.0.0-beta.4 round 3 (schema 1.0.51, #1347, #1340)#109
Open
krukow wants to merge 9 commits into
Open
Upstream sync: post-v1.0.0-beta.4 round 3 (schema 1.0.51, #1347, #1340)#109krukow wants to merge 9 commits into
krukow wants to merge 9 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Syncs the Clojure Copilot SDK with upstream CLI schema 1.0.51 and ports a small set of post-v1.0.0-beta.4 Node SDK behavioral changes, keeping generated wire specs aligned while updating public-facing behavior/docs/tests.
Changes:
- Bump pinned schema version to
1.0.51and regenerategenerated/event_specs.clj(includingttftMs→timeToFirstTokenMsand bounded ints tointeger?). - Update
pingto reflect:timestampnow being an ISO-8601 string (tests, mock server, docs). - Make MCP stdio
:mcp-argsoptional; add SessionFs SQLite support plumbing and wire “opaque field” preservation for sqlite rows/bind params.
Show a summary per file
| File | Description |
|---|---|
| test/github/copilot_sdk/mock_server.clj | Mock server updates for ping timestamp shape and sessionFs.setProvider RPC handling. |
| test/github/copilot_sdk/integration_test.clj | Updates ping assertions and adds SessionFs SQLite + MCP :mcp-args optionality tests. |
| test/github/copilot_sdk/e2e_test.clj | Updates E2E ping assertions for string timestamp. |
| src/github/copilot_sdk/specs.clj | Adds SessionFs SQLite/provider capability specs and makes MCP :mcp-args optional; adds :time-to-first-token-ms. |
| src/github/copilot_sdk/session.clj | Implements SessionFs SQLite adapter/dispatch support and validates sqlite capability vs handler availability. |
| src/github/copilot_sdk/protocol.clj | Preserves opaque sqlite bind params on ingress and sqlite row keys on egress. |
| src/github/copilot_sdk/generated/event_specs.clj | Regenerated wire event specs for schema 1.0.51 (TTFT rename, integer bounds, enums, etc.). |
| src/github/copilot_sdk/client.clj | Forwards SessionFs capabilities on sessionFs.setProvider; updates ping docstring and request routing for sqlite RPCs. |
| src/github/copilot_sdk.clj | Updates public ping docstring to reflect timestamp change. |
| schemas/session-events.schema.json | Updates vendored upstream schema content (enums, integer bounds, TTFT rename, etc.). |
| schemas/README.md | Updates pinned schema version display to 1.0.51. |
| README.md | Bumps documented dependency version to 1.0.0-beta.4.1. |
| examples/permission_bash.clj | Switches deprecated permission result kind to :approve-once. |
| doc/reference/API.md | Documents optional SessionFs sqlite keys/capability behavior and round-tripping semantics. |
| doc/mcp/overview.md | Documents :mcp-args as optional for stdio MCP servers. |
| CHANGELOG.md | Adds round-3 unreleased entries for schema bump and behavior changes. |
| build.clj | Version bump to 1.0.0-beta.4.1. |
| .copilot-schema-version | Pins schema version to 1.0.51. |
Copilot's findings
- Files reviewed: 17/19 changed files
- Comments generated: 1
Pin .copilot-schema-version to 1.0.51 (latest GA on npm), re-fetch schemas/, and regenerate src/github/copilot_sdk/generated/event_specs.clj via 'bb codegen'. Notable generated diffs: - :ttft-ms removed, :time-to-first-token-ms added in assistant.usage-data (wire field 'ttftMs' → 'timeToFirstTokenMs' upstream). - Many predicates change from number? → integer? (upstream PR #1329 uses 32-bit types for bounded schema integers). - Cosmetic gensym renumbering on anyOf-derived specs. No new event types, no new top-level event-data fields beyond the rename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Upstream PR #1347 made MCPStdioServerConfig.args optional across all
SDKs. Move ::mcp-args from :req-un to :opt-un on ::mcp-local-server
(aliased as ::mcp-stdio-server). Add red-then-green test asserting
{:mcp-command "true" :mcp-tools ["read"]} validates with no :mcp-args.
Update doc/mcp/overview.md to mark :mcp-args optional and document the
CLI 1.0.51 origin.
Also extend ::assistant.usage-data to accept :time-to-first-token-ms
(new wire name from CLI 1.0.51 schema rename of ttftMs →
timeToFirstTokenMs) alongside legacy :ttft-ms, so events from older
and newer CLIs both validate. Add a focused test for both shapes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CLI 1.0.51 changed the 'ping' RPC result timestamp field from epoch millis number to an ISO 8601 date-time string. The SDK forwards the field verbatim, so callers of sdk/ping now receive a string :timestamp. - Update the mock test server to return (.toString (Instant/now)). - Update test-ping and ^:e2e test-e2e-connection assertions to expect a string, and also parse it as Instant to assert ISO 8601 shape. - Update docstrings on sdk/ping and client/ping to document the new shape and note that earlier CLI versions returned a number. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document the sync of CLI schema 1.0.49 → 1.0.51, including the three behavioural changes ported (PR #1347, the ttftMs rename, and the ping timestamp shape change) and the items deliberately tracked but not ported (PR #1316 packaging-only, PR #1327 ToolBinaryResult tighten). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
krukow
force-pushed
the
upstream-sync/post-v1.0.0-beta.4-round-3
branch
from
bbc4fe3 to
ae7e6f6
Compare
Add explicit 'sync local main first' step in Phase 1 (Discovery) and Phase 8 (PR Creation) of the update-upstream skill. A stale local main caused this round-3 PR to be rebased post-hoc against origin/main after PR #108 (round 2) was merged. Also document the post-hoc remedy: when an already-squash-merged commit shows up during rebase, 'git rebase --skip' is the correct action and Git's 'patch contents already upstream' detection will handle subsequent duplicates automatically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- ::specs/timestamp now accepts both ISO 8601 strings (CLI ≥ 1.0.51) and numeric epoch-millis (older CLIs), making the spec consistent with the ping docstring contract. The ping fdef return spec no longer fails instrumentation against older CLIs. - Fix "Reflecing" → "Reflecting" typo in update-upstream SKILL.md. - Add backward-compat assertion for legacy :ttft-ms key in test-assistant-usage-time-to-first-token-ms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+27
to
+29
| ;; and numeric epoch-millis (older CLIs). Used by event timestamps and ping | ||
| ;; results. Conversion to java.time.Instant happens at the coercion layer for | ||
| ;; events; ping forwards the server value verbatim. |
Collaborator
Author
There was a problem hiding this comment.
Valid — fixed in 4ff5197. Reworded to clarify that the SDK forwards :timestamp verbatim and consumers needing java.time.Instant should parse it themselves. The coercion layer only transforms a curated set of event-data fields, not the envelope timestamp.
Comment on lines
+27
to
+30
| ;; and numeric epoch-millis (older CLIs). Used by event timestamps and ping | ||
| ;; results. Conversion to java.time.Instant happens at the coercion layer for | ||
| ;; events; ping forwards the server value verbatim. | ||
| (s/def ::timestamp (s/or :iso-string string? :epoch-ms number?)) |
Collaborator
Author
There was a problem hiding this comment.
Valid — tightened to nat-int? in 4ff5197. Epoch-millis are inherently non-negative integers; number? was overly permissive and would have allowed nonsensical values like -1.5 past the spec.
| |-----|------|----------|-------------| | ||
| | `:mcp-command` | string | Yes | Command to execute | | ||
| | `:mcp-args` | vector | Yes | Command arguments | | ||
| | `:mcp-args` | vector | No | Command arguments (optional since CLI 1.0.51; omit for argument-less commands like `true`) | |
Collaborator
Author
There was a problem hiding this comment.
Valid — removed the true example in 4ff5197. The wording now just says "omit for argument-less commands" without naming a specific binary, which avoids the Windows portability concern.
- ::timestamp epoch-ms branch is now nat-int? (epoch-millis are non-negative integers, not arbitrary numbers). - Fix misleading comment that implied event :timestamp is coerced to Instant at the coercion layer; actually only certain event-data fields are coerced. - Drop POSIX-specific 'true' example from MCP overview (not portable to Windows). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ;; and numeric epoch-millis (older CLIs). Used by event timestamps and ping | ||
| ;; results; the SDK forwards the server value verbatim. Event consumers that | ||
| ;; need a java.time.Instant should parse the string form themselves. | ||
| (s/def ::timestamp (s/or :iso-string string? :epoch-ms nat-int?)) |
Collaborator
Author
There was a problem hiding this comment.
Not a valid concern — nat-int? in Clojure accepts longs. It is built on int? (= (or (instance? Long x) ...)), not on Java's 32-bit Integer. Verified at the REPL:
(nat-int? 1700000000000) ;; => true
(nat-int? Long/MAX_VALUE) ;; => trueThe source is:
(defn nat-int? [x] (and (int? x) (not (neg? x))))So System/currentTimeMillis values (which are long) validate fine. The reviewer likely confused this with Java's int/Integer (32-bit), but Clojure's int? predicate is name-overloaded and actually means "fixed-precision integer" (Long, Integer, Short, Byte, BigInteger). No change needed.
Comment on lines
+7
to
+13
| - **`ping` `:timestamp` is now an ISO 8601 string** — Upstream PR #1340 | ||
| (CLI 1.0.51) changed the `ping` RPC result `timestamp` field from | ||
| epoch-millis number to an ISO 8601 date-time string (e.g. | ||
| `"2026-05-21T08:00:00.000Z"`). The SDK forwards the field verbatim, so | ||
| callers of `sdk/ping` now receive a string `:timestamp`. The mock test | ||
| server was updated to match. The ping docstring documents the change and | ||
| notes earlier CLI versions returned a number. (upstream PR #1340) |
Collaborator
Author
There was a problem hiding this comment.
Valid — reworded in 4d7bf53. The bullet now makes clear that CLI 1.0.51 changed the field type, and that the SDK accepts both shapes via ::specs/timestamp so callers see a string against CLI ≥ 1.0.51 and a number against older CLIs.
Reword the round-3 'Changed' bullet to make clear that CLI 1.0.51 (not the SDK) changed the ping :timestamp field type, and that the SDK accepts both shapes via ::specs/timestamp so callers see a string on CLI ≥ 1.0.51 and a number on older CLIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ;; to ISO 8601 string (`timestamp: string, format: date-time`). | ||
| (is (string? (:timestamp result))) | ||
| (is (some? (java.time.Instant/parse (:timestamp result))) | ||
| ":timestamp parses as ISO 8601 instant")))) |
Collaborator
Author
There was a problem hiding this comment.
Valid — added in the latest commit. test-ping now includes a testing block that asserts ::specs/timestamp validates an ISO string, a System/currentTimeMillis long, and a representative epoch-ms long (1700000000000), and rejects -1 and 1.5. This locks in the backward-compat contract independently of the mock server's actual response shape.
…dates Add explicit assertions that ::specs/timestamp accepts both an ISO 8601 string and a System/currentTimeMillis-sized long, plus boundary checks (negative rejected, fractional number rejected). Locks in backward compatibility with older CLIs that still return numeric :timestamp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
Upstream sync from CLI schema
1.0.49→1.0.51, bringing the Clojure SDK to Clojure version1.0.0-beta.4.1.The upstream Node.js SDK has no new release tag past
v1.0.0-beta.4, but 11 commits tonodejs/src/have landed since. This PR ports the behavioural changes (3 SDK-visible items) and absorbs the schema bump viabb codegen.Changes
Schema bump 1.0.49 → 1.0.51 + regenerate event specs
.copilot-schema-versionto1.0.51.schemas/and regeneratesrc/github/copilot_sdk/generated/event_specs.clj.ttftMs→timeToFirstTokenMs(:ttft-ms→:time-to-first-token-ms).number?→integer?(upstream PR #1329).MCP stdio
:mcp-argsis now optional (upstream PR #1347)::mcp-local-server: move::mcp-argsfrom:req-unto:opt-un.ping:timestampis now an ISO 8601 string (upstream PR #1340)(.toString (Instant/now)).Instant.sdk/ping/client/ping.CHANGELOG + version bump
1.0.0-beta.4.1(UPSTREAM.CLJ_PATCH).Tracked-but-not-ported
index.ts) — Node.js packaging only; Clojure already exposes these viagithub.copilot-sdk.generated.event-specs.ToolBinaryResult.typetightened — our::binary-results-for-llmis intentionally permissive. Deferred.Validation
bb test(unit + integration)bb testwithCOPILOT_E2E_TESTS=truetest-e2e-blob-attachmentLLM timeout, unrelated; passed on re-run)./run-all-examples.shbb validate-docsbb jarcopilot-sdk-clojure-1.0.0-beta.4.1.jarCode review
Two parallel
code-reviewagents (Claude Opus 4.7 and GPT-5.5) reviewed the diff. Both reported no significant issues.Rubber-duck assistance
The rubber-duck reviewer caught the
ping:timestampshape change (PR #1340) as a missed item in my initial scope — I had assumed the SDK didn't surface it, butsdk/pingis a public function and existing unit/E2E tests asserted(number? (:timestamp result)). Added to scope and ported.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com