feat: sessions and files namespaces (3/5) - #10
Merged
Conversation
First slice of milestone 3. Types only: the 28 schemas the session, agent and file routes reach, derived from the description and brought under the conformance gate. No methods yet. The gate earned its keep immediately. Adding these types surfaced 14 enumerated fields across the mapped set, where enums.go named only 6 — including MessageDataRoleAssistant, which the driver's production code compares against. enums.go now names all 49 declared values. Three roots the routes need are absent from the description. SessionCreateRequest is hand-written because the create route takes a raw body and dispatches on Content-Type, so FastAPI emits no requestBody for it. SessionFile and RunnerInfo are hand-written because those routes declare no response shape at all: GET, POST and DELETE on the session files collection all publish an empty schema, and GET /v1/runners publishes a free-form map of arrays of untyped objects. That means the whole file surface sits outside the conformance gate, in both directions. Upstream has the same problem and solves it the same way — its _files.py converts a raw dict by hand. doc.go names each such contract, and the methods that reach them land in the next slice.
TestEveryMirroredTypeIsMapped checked reachable-implies-mapped. Nothing checked the inverse, so an entry in schemaFor could name a schema for a type no root walks, and the field, type and enum checks would all skip it in silence. Review named this gap after two dead entries sat there unnoticed; adding the session types re-created it at scale — 28 mapped and 0 of them inspected. mirroredTypes derived its roots from eventRegistry alone, which reaches only what the stream decodes. The request and response surface needs its own roots. Eleven of them are the ConversationItem.Data union: the description declares an eleven-variant anyOf, Go has no sum type, so the field decodes as any and a caller unmarshals into one of these after switching on the item type. Nothing else reaches them. Coverage goes from 65 types and 271 fields to 93 and 491, which is every entry schemaFor names. Verified by removing one root and confirming the new test fails on it.
Three blinded reviewers. Idiom and prose dissented, structure ratified.
The gate had a hole exactly where this PR made its choices. goKindFor mapped
"object" to reflect.Map and stopped, so map[string]any satisfied a schema
declaring additionalProperties as a $ref or a typed value. Ten fields sat in that
blind spot, nine of them new here. The type test now descends one level into
additionalProperties and items, and the ten are typed: Labels and LastTaskError
and Headers to map[string]string, MCPStartup to map[string]MCPServerStartup,
UsageByModel to map[string]ModelUsage. Extending the gate first meant the fixes
landed enforced rather than merely applied.
Two exported names contradicted the package's own initialism table, which lives
in code at conformance_test.go: LlmModel where goFieldName returns LLMModel, and
TotalCostUsd where the same wire key is already TotalCostUSD three times over. My
earlier initialism sweep ran before this file existed, so it found nothing.
ConversationItem.Data was any. The description gives it eleven $refs and no
discriminator, so a sealed interface like Event would need a variant map no test
could pin — the category doc.go warns about. It is json.RawMessage instead, the
shape APIError.Detail already sets, and the variant list and switch key now sit on
the field rather than only in a test comment.
UpdateSessionRequest.TerminalLaunchArgs carried omitempty, which drops an empty
slice, so a caller could not send the [] the description defines as "replace
wholesale" — and the stored value in the example is a permission-skipping flag.
Without omitempty, nil marshals to null (leave unchanged) and []string{} to []
(clear). project_id deliberately keeps omitempty: an explicit null is a 400 there.
Thirteen pointer fields documented a value unconditionally — Deleted *bool as
"Always true." on the only confirmation field of a delete. Each now leads with
when it is nil. PaginatedList.Data justified itself with "list is invariant",
which is a statement about Python, and named two types this package does not
declare.
Five of fourteen enum doc comments ended mid-sentence and two split a hyphenated
word across a line break, which go doc publishes as "sub- agent". The generator
truncated at three wrapped lines regardless of where the sentence ended; it now
takes whole sentences and never breaks inside a hyphenated token. A missing blank
line had also fused the file comment onto the first const block, scoping the
switch-safety rule to two constants out of forty-nine; that rationale now lives in
doc.go where a reader is already looking.
NOTICE named types.go and event.go while session_types.go reproduces 144 upstream
descriptions and enums.go nine. AGENTS.md had said that list was "exhaustive by
measurement, not by convention. Nothing checks it for you" — and the next commit
proved it. TestNoticeNamesEveryFileCarryingUpstreamProse is the something that
checks it, in both directions.
doc.go claimed there is no session surface, and both doc.go and AGENTS.md still
said four conformance tests when there are six over five dimensions. The
conformance header listed three of the five.
Structure, from the brandon-code pass: freeFormFields was an escape hatch with no
entries and no caller, so it is gone and its knowledge sits on goKindFor.
TestEveryUnionMemberDecodes decodes nothing and is now
TestEveryUnionMemberIsRegistered. The two field tests had drifted on the tag rule
— one guarded an empty name, the other did not — so both call one wireName.
Dropped one history sentence and one count that was already stale.
The session surface as a namespace, matching upstream: Client.Sessions() rather than twenty-odd more methods on Client. Create, Get, Delete, Fork, Update, the six named patch wrappers, PostEvent and its SendMessage/Interrupt/Compact wrappers, ResolveElicitation, and four listings. Every listing pages internally and yields one sequence, so a caller ranges once instead of writing the same cursor loop. pageSeq stops on an empty cursor as well as on has_more, because a proxy that reports more while returning nothing would otherwise loop without end. It starts no goroutine, so abandoning the range issues no further request — tested. Three things the description corrected, each of which my own plan had wrong. Clearing a model override or a reasoning effort uses an alias the server reads, not the empty string, so SetModelOverride now rejects "" and points at ClearModelOverride. And there is no UnbindRunner: the description says only that a nil runner_id leaves the binding unchanged and defines no value that releases one, so the method is absent with a comment saying why rather than shipped on a guess. ChildrenTree bounds three ways, each for a failure it would otherwise hit: depth, because a caller cannot know a server-shaped tree's depth; concurrency, because one listing per child per level is a request storm; and a visited set, because a tree carrying a cycle never ends. A repeat is recorded as a truncated leaf rather than followed, so the topology stays honest. Every child pages, which is where upstream's own walk stops short. Bounded with a channel semaphore rather than errgroup, because the module has no dependencies and keeps none. SubtreeBusy reports an error rather than a false negative when the walk truncated: a quiet answer from a walk that stopped early is not evidence of a quiet subtree. A positive answer is returned as-is, because it is sound whatever the walk missed. Files is session-scoped only. Upstream keeps a flat half — files.get(file_id) and friends — but every one of those raises: /v1/files was removed server-side, and the description carries no non-session file route. Porting a method that cannot work would be porting a tombstone. Upload takes an io.Reader and streams through a pipe, so memory does not track file size — measured at 32 MiB. The writing goroutine closes the pipe on every path, so a failing reader surfaces as an error rather than a hang. Download requires an explicit byte bound and reads one byte past it, because stopping exactly at the limit cannot tell a file that fits from one that was truncated. doc.go now names the four hand-written contracts this milestone reaches and where each is used, since the file surface sits wholly outside the conformance gate.
The last two of upstream's session methods, and the tests the surface was short of. ResolveAgent closes the gap between what a person picks and what session creation binds: a picker lists names, Create takes an id. It follows the listing cursor, so an agent past the first page resolves. A miss wraps ErrNotFound — no 404 was involved, the listing succeeded and carried no such name — and names some of the names it did see, capped at fifty, because a large deployment should not build thousands to render one error. ResolveOnlineRunner finds a runner that can drive a harness. A runner reporting no harness list is the fallback, taken only when nothing advertises the harness outright: the server matches the same way, and treating silence as a refusal leaves a usable runner idle. Canonicalize is a function rather than a table because the aliases are the server's, and a table here would be a second copy going stale. No match is an empty id and a nil error, because that is a normal state a caller waits on rather than a failure. RunnerInfo is hand-written. GET /v1/runners publishes a map of arrays of untyped objects, so nothing pins these fields and a rename breaks the resolve silently. The new tests target what a hand-written client actually gets wrong. A conformance gate cannot see a wrong route or a wrong verb, because neither is a type, so eighteen table cases assert method, path and body for every call — including that BindRunner patches rather than posts, and that Interrupt and Compact are inputs on the events route rather than routes of their own. Options are asserted at the wire, not on the struct, since an option that never reaches a header is the failure worth catching. And the base-URL redaction is tested on both paths where a password can hide, including url.Parse's own error, which quotes its input. Coverage 71% to 87.1%. Excluding the 53 sealed-interface markers — one statement each, which no honest test calls — every one of the 98 remaining functions has coverage. Reporting it both ways because the marker count moves the headline number without moving what is tested.
Findings from four blinded reviewers. The two unbounded-resource defects were reachable today and neither the suite nor 89% coverage on ChildrenTree saw them, because the tests exercise statements and the requirements are about properties. A listing never ended. pageSeq stopped on has_more and on an empty cursor, and on nothing the server could not control: a listing returning a cursor it had already returned paged forever. Measured at 26,094 requests in two seconds, with ChildrenTree buffering every row before its visited set could dedup, so the heap grew with them. It now stops on a repeated cursor and at a page ceiling, and says which cursor repeated. The comment claiming the old pair made the walk "terminate on its own rather than on the server's good behaviour" was false; it closed one case of three. Upload leaked its writer goroutine. The goroutine starts before doUpload validates anything, so a rejection that never builds a request left nothing to close the pipe and the goroutine blocked writing the multipart header forever — reachable through a session id the request path rejects. Measured: three rejected uploads, three permanently blocked goroutines. One deferred Close on the read half fixes it. The invariant comment named the wrong side of the pipe: the request side was never the side that hangs. The concurrency bound skipped the widest level of the walk. The depth-cap probe issued a listing per child without taking a token, and that level has more requests than every other combined. A bound of 4 admitted 28 in flight, measured. It now takes the same token as a descending listing. Download wrote one byte past the caller's bound. Reading one past is how a file that fits is told from one truncated, but the byte travelled through the caller's writer rather than a probe, and the returned count was clamped so it disagreed with what the writer received. It now copies exactly maxBytes and probes the body separately. The old test passed throughout, because it asserted the clamped return value and never looked at the sink; it is replaced by one that checks both. Two sentinels replace wrong ones. ErrTruncated for a response past the caller's bound, and ErrListingUnbounded for a listing that never ends — both were ErrInvalidArgument or stream vocabulary, and doc.go defines ErrInvalidArgument as meaning this package rejected the call before sending anything. Neither case did. SubtreeBusy reports completeness as a value rather than an error. Truncation is a property of the answer, not a failure: a caller raises MaxDepth or accepts the bound, and forcing that through an error made the documented zero-value call fail on any tree deeper than three. It also now says plainly that it answers about the subtree below the named session, because no listing describes the named session itself and its state was never read. SessionFile.Raw is populated. It was documented in two places as the escape hatch for the one surface with no schema, and nothing filled it, so a caller reaching for an unnamed server field got nil. UnmarshalJSON now keeps the body. Also names recordErr in the walk, where the first-error-wins dance was written twice.
…rder The visited set was global, so a session reachable through two parents was claimed by whichever goroutine won the race, and the claiming node's depth then decided how much of that subtree the depth cap admitted. Review measured the same fixture answering (true, nil) and (false, truncated) with 60ms of added latency between runs. A busy signal that changes with network jitter cannot drive control flow. The set is now per path, copied on descent. A path cannot race with itself, so the answer follows the tree and nothing else. Cycles are still caught, because a cycle repeats within a path. The cost is that a session reachable two ways is expanded under both, which is true to the topology and which MaxNodes bounds. MaxNodes is that bound, defaulting to a thousand. The walk spawns one goroutine per node, so before this the tree's shape decided the process's memory: review measured 512 goroutines at Concurrency 2, and branching 500 at depth 3 wants 125 million. Measured now: a width-500 tree at depth 3 peaks at about a thousand goroutines and reports the rest as truncated rather than attempting them. The goroutine-per-node shape remains; a worker pool draining a node queue would hold the same request profile at a fixed count, and that is a larger change than this. TreeNode.Truncated was answering three questions at once. A node with no children now says which of four reasons applies: it has none, the walk stopped at a bound, the session already appears on the path above, or its own listing failed. Err is new and closes the finding that an unreadable subtree was indistinguishable from an empty one — a UI rendered a failure as "no children". SubtreeBusy accounts for those separately. Truncated and Err make the answer incomplete; Repeated does not, because a repeated session's subtree was already walked where it first appeared. The determinism test needed two attempts. The first fixture put both parents at depth 1, which is symmetric, so it passed against the defect it was written to catch. The reachability has to be asymmetric — depth 1 through one parent and depth 2 through the other — for the claiming node's depth to matter. Verified by restoring the shared set and watching the shallow-path case report false.
Splitting TreeNode.Truncated into Truncated, Repeated and Err left this test asserting the old overloaded meaning, so it failed on the commit that split them. It now checks Repeated and that Truncated is absent, which is the distinction the split exists to make. The commit that split the field pushed with the suite red. check.sh reported it and I committed in the same step rather than gating on it.
Four deferred findings whose stated un-defer conditions are now met, plus the sanitizer coverage this package already required of itself. Sanitize every server-chosen field. sanitizeForError existed and two new call sites did not use it: APIError.Code (rendered when Title is absent) and APIError.RequestID both reached the message raw, and ResolveAgent rendered server-chosen agent names unbounded. Measured before: a 400 with a CRLF in Code forged a second log line, and a 1000-agent listing produced a 40KB two-line error. After: one line, 450 bytes, truncation reported. Adds maxRequestIDRunes and maxAgentNameRunes, and caps the rendered name list by total length rather than count alone. Reject a filename that would forge a multipart header. mime/multipart escapes a quote and a backslash and passes CR and LF through, so a CR in a filename ends the Content-Disposition line and what follows is read as another header or part. Proven against mime/multipart directly. Callers derive filenames from a model, so this is reachable input. Give a transfer its own budget. doUpload and doDownload charged a whole file to the 90-second unary timeout, so a large upload could not succeed at any link speed and a download truncated mid-body. Measured against a 200ms budget: the upload died with 4267 of 10240 bytes sent and the download at 5120 of 10240, both while making steady progress. Transfers now run on a third http.Client with no whole-exchange bound; the context is the limit, and WithTransferTimeout sets one ceiling for callers who want it. Unary calls keep theirs. Gate the subtree walk per request, not per listing. Concurrency bounded listings, so a node paging many times held a slot for its whole drain and a branch whose levels are sequential round trips waited behind it: a fixture with four multi-page nodes and a chain took 770ms against a 500ms critical path, and 576ms after. gateFetch carries the contract and honours ctx while waiting. Classify a redirect an upload cannot follow. net/http follows a 307 or 308 by replaying the request, and a streamed body cannot be replayed, so it returns the response and checkRedirect never runs. An upload reported a bare 307 for both a hostile location and a same-host path rewrite — the second being what a path-rewriting proxy does. checkRedirect's own gates now decide which: ErrUnsafeRedirect off-host, ErrRedirectNotFollowed on-host with the one fix named. The credential still never travels. Also: reattach checkRedirect's doc comment, which was fused onto stripRedirectURL and left the redirect policy undocumented; correct ChildrenTree's "visits each session once", which the per-path visited set reversed; and replace the upload heap-growth test with a rendezvous. That test read a process-wide heap while running in parallel, so its verdict depended on what else the suite was doing, and it tipped once here. The rendezvous is deterministic: the reader withholds its second chunk until the server has read part of the first, which a buffering implementation cannot satisfy. Every gate is mutation-proven, with the mutation asserted as landed. Deferred: the goroutine-per-node fan-out, which MaxNodes bounds; un-defer at the first tenant with more than roughly 200 children at one level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review round's remaining merge conditions, each found by scanning rather than from a list. Reattach and retarget the links. checkRedirect's twenty-line security rationale was fused onto stripRedirectURL, so the redirect policy itself was undocumented. Six links named nothing: two constructors that do not exist, and four methods attributed to Client when they are on Sessions. TestEveryDocLinkResolves now fails the suite on either shape, and checks a member against the member rather than accepting its owning type — the wrong-owner link is the costly one, because it reads as authoritative. Its two skip rules are structural, so it needs no allowlist to rot. Delete an orphaned doc comment. list.go carried thirteen lines describing ListAgents as returning one page and saying no lookup-by-name route exists. It attached to nothing, and both claims had become false: the method walks every page, and ResolveAgent is the lookup. What was still true moved onto the real method, where go doc publishes it. Attach the rest, or stop repeating them. Four more blocks rendered nowhere: the patch-wrapper rationale now sits on Update, and "there is no UnbindRunner" on BindRunner, which is where a reader goes looking for it. The two that restated doc.go are now three lines pointing at the section that owns the policy — and the section names were checked, because a pointer that dangles is worse than the duplication. Correct doc.go. Scope still said no method reaches the session routes, which PR 3's twenty-seven methods contradict. Timeouts said two clients, which the transfer client makes three. The optional-field rule claimed every optional field is a pointer; eighteen are not, and eleven of those are request fields where omitempty on a plain value is the right idiom. The rule now says which direction it governs, so the next reader does not "fix" the eleven. Unexport TerminalTaskStatuses. An exported slice is mutable by every importer, and appending to it would change what SubtreeBusy reports for every caller in the process. IsTerminalTaskStatus was already the way to ask. Make SessionFile's optional fields pointers, as this package's own rule requires. Bytes conflated a zero-length file with one the server did not measure, which its own comment admitted. Also: drop history narration from two comments, and replace AGENTS.md's "nothing checks it for you" about NOTICE — something does, and the rule now names the test instead of restating a list that had already gone stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up PR 2's removal of docs/adr/0001-rebuild-rather-than-reland.md, which landed after this branch started. doc.go conflicted because both sides rewrote the Scope paragraph: PR 2 dropped the ADR pointer, this branch replaced "there is no session, files ... surface" with what the two namespaces now carry. Resolved by keeping this branch's text without the pointer, since the file it named is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up the Go floor bump to 1.25.0, the Event interface change, the lint-job toolchain pin, and PR 2's own docs corrections. Five conflicts, resolved on the merits rather than by side: - NOTICE and the conformance-test count: this branch's numbers are the accurate ones, because it adds session_types.go and a sixth conformance test. - doc.go's Cancellation note: PR 2 said this release cannot post an interrupt, which was true there. Sessions.Interrupt exists here, so the pointer stands. - attribution_test.go and doc_links_test.go: both branches added them. Took PR 2's copies — its attribution doc states the present rather than narrating the change that prompted it, and its doc-link example names no type, so it stays true in either branch. - AGENTS.md's NOTICE rule: kept PR 2's "exhaustive by measurement, not by convention" framing with this branch's note on what failing looks like. Also applies t.Context() to the 59 sites added on this branch, so the convention holds across the whole suite rather than only the files PR 2 touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…to feat/alignment-03-sessions
Collaborator
Author
Reviewer note: the wire types are likely to become generatedA spike after this PR was opened showed that Two consequences for reviewing this PR. Review the shape, not the authorship. Because the generated types are
The proposal is written up as an ADR at status Proposed, with the Deliberately not in this PR. Generation deletes One related fact worth knowing while reviewing: the vendored spec is one event |
bdchatham
changed the title
schemaFor held 94 rows and 87 of them said a name equals itself. Those rows carried no information, and every rename had to touch two places to stay true. A Go name is its schema name. Seven types differ: two because Go writes an initialism in capitals, five because this package prefixes a response event the description leaves bare, so ResponseCompletedEvent does not collide with TurnCompletedEvent. Those seven are the whole table now. TestEveryMappedTypeIsReachable and TestEveryMirroredTypeIsMapped go with it. Both existed to police the hand-maintained list in each direction, and a name that defaults to itself cannot disagree with the set of names. What they were really guarding is unchanged: declaredProperties fails when a type resolves to a schema the description does not declare, and surfaceRoots still decides what gets walked at all. Verified in both directions: an exception pointing at a schema that does not exist fails, and removing a needed exception fails with `spec declares no schema "ResponseCompletedEvent"`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three passages named a count and a dimension the previous commit removed. The mapping-complete-in-both-directions dimension went with the two tests that policed the hand-maintained table, so "five dimensions" described four and "six tests" described three. Each now states the list and no number. A count in prose goes stale the first time a test is split or retired, and it went stale twice while this branch was open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Correction to my earlier note:
|
amir-deris
approved these changes
Aug 21, 2026
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
2 participants
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.
Milestone 3 of
specs/002-upstream-alignment. Adds the two namespaces thatmirror upstream's
client.sessionsandclient.files.for_session, so the Gosurface reads the way the Python client does.
Stacked on #8. Review that first; this diff is against its branch.
Shape
Filesis split fromSessionsrather than hung off it, matching upstream: threeof its operations do not need a session id, and forcing one on them was wrong.
session_types.go, pluscontracts.gofor theroutes the description does not declare.
iter.Seq2over a pager that refuses to run forever: a pageceiling and a repeated-cursor check, both named in the error.
Sessions.ChildrenTreewalks a subtree under three caller-set bounds(depth, in-flight requests, node count) and reports why it stopped per node.
Decisions worth a look
ConversationItem.Dataisjson.RawMessage. The description gives the itempayload 11
$refs and no discriminator, so a variant map cannot be pinnedwithout guessing. Raw bytes are honest; a sealed interface would not be.
There is no
UnbindRunner. The description says a nilrunner_idleaves thebinding unchanged and defines no value that releases one. Noted in the source
next to
BindRunnerso the absence reads as a decision, not an oversight.SessionFilekeeps its raw body. Every file route publishes an emptyresponse schema, so the named fields are observed rather than guaranteed and
Rawis the escape hatch.Fixed here, each with a measurement
Uploadleaked a goroutineDownloadoverran the sinkbusyX-Injectedlanded307either wayEvery gate is mutation-proven, with the mutation asserted as landed.
Verification
bin/check.shrc=0 ·-race -count=2clean ·golangci-lint0 issues ·103 tests · 88.6% coverage · zero module dependencies.
Known gaps
MaxNodesbounds it;a worker pool waits for a tree with ~200 children at one level.
is a scheduling race, so a timing assertion passes on a lucky schedule. The
mechanism (
gateFetch) is tested; the emergent behaviour is measured only.