Polish batch: lifecycle, discovery version semantics, handle docs, putAttachment return shape - #155
Merged
Merged
Conversation
Teardown was two calls in a documented order, and correctness silently depended on each adapter's close() happening to be flush-inclusive — none of them are. Fold the flush into close() so the ordering is guaranteed once at the invariant layer, and drop the now-redundant flush() from the quick starts. The flush runs in a try/finally: an unwritable stack must not also leak a lock file. close() is idempotent because adapters aren't independently required to tolerate a double close (node:sqlite throws on an already-closed handle, and lock release is not re-entrant). Also corrects flush()'s doc comment, which described an offline write queue the API adapter does not have. Refs #147 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019eqUFNRUhV3x5JrjkMTpxj
close() left a flag that gated only itself; every other method carried on into a dangling adapter handle. The failure surfaced as whatever the engine said about it — node:sqlite's ERR_INVALID_STATE, or silence on an adapter that accepts writes it will never persist. Guard every public method on Stack, plus the one ScopedStack path that reaches the adapter without going through Stack first, so a closed stack writes no attachment bytes before refusing. StackClosedError stays outside the StackError taxonomy, alongside IdGenerationError and InvalidDidError: every StackError maps to a wire status, and no server responds with "your client is closed". flush() throws like any other operation; only close() is idempotent, since a caller cannot always know whether teardown already ran. Refs #147 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019eqUFNRUhV3x5JrjkMTpxj
Discovery has always carried a "version" field that nothing read and no rule governed, so a server could ship one meaning anything and a client could do nothing with it. Give it semantics: MAJOR.MINOR of the wire protocol itself, majors must match, minors never have to. A major bump is defined as a change that would make an older client read a response wrongly, which is precisely what makes refusal the only safe response; a minor is additive either way, and neither direction can misread. Missing or unparseable is refused too — the field is mandatory, so its absence is a server not implementing this spec. APIAdapter.open() applies the rule before any other request, so a caller never has to wonder which writes landed against a server it cannot speak to. DiscoveryResponse moves to wire-types alongside the constant and the comparison, so a server implementation shares them rather than reimplementing the rule from prose. Refs #147 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019eqUFNRUhV3x5JrjkMTpxj
Both handle fields opened with "Short unique identifier", so a reader who stopped at the first three words got a guarantee that does not exist. State what the field is instead of qualifying a misleading phrase. Uniqueness is unenforced deliberately, not pending: the petname model makes global uniqueness incoherent, per-stack uniqueness adds nothing over the DID that already identifies the profile, and nothing in the library resolves an entity or group by handle at all. The reasoning goes in the spec, with the field comments pointing at it. Also records that handle lookup, for an app that wants it anyway, rests on contentFieldQuery — a capability a server may decline. Refs #147 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019eqUFNRUhV3x5JrjkMTpxj
The uploader got back only a fileId, so the record they had just created was
the one thing they could not address — and filename, its only mutable field,
needs an id to set. Getting one meant querying by fileId and disambiguating
among the several records a shared fileId can have.
Return the record instead, matching what POST /attachments returns on the
wire and what create() already returns. Every path had it in hand and was
discarding it: the atomic path takes the server's response, and both
fallback paths take what their own create() produced.
BREAKING CHANGE: Stack.putAttachment(), ScopedStack.putAttachment(), and
StackClient.putAttachment() return StackRecord & { content: AttachmentContent }
instead of the fileId string. Callers wanting the id read content.fileId.
The adapter-level StackBlobAdapter.putAttachment() is unchanged — it remains
the bytes-only primitive returning a FileId.
Refs #147
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019eqUFNRUhV3x5JrjkMTpxj
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
The four items from #147, one commit each, plus a fifth that fell out of the first.
close()impliesflush()(1ae3671) — teardown was two calls in a documented order, and correctness silently depended on each adapter'sclose()happening to be flush-inclusive. None of them are. The flush now runs insideclose(), in atry/finallyso an unwritable stack doesn't also leak a lock file, andclose()is idempotent because adapters aren't independently required to tolerate a double close (node:sqlitethrows on an already-closed handle; lock release isn't re-entrant). Also correctsflush()'s doc comment, which described an offline write queue the API adapter does not have.Refuse work on a closed stack (
a1d8ec9) — commit 1 introducedclosedstate that gated onlyclose()itself, leaving a half-built state machine. Every other public method now throwsStackClosedError, which sits outside theStackErrortaxonomy alongsideIdGenerationErrorandInvalidDidError: everyStackErrormaps to a wire status, and no server ever answers "your client is closed."Discovery version negotiation (
52433e5) —versionwas a field nothing read and no rule governed. It now meansMAJOR.MINORof the wire protocol: majors must match, minors never have to, and missing or unparseable is refused the same as a mismatch.DiscoveryResponse, the constant, and the comparison moved to@haverstack/wire-typesso a server implementation shares them instead of reimplementing the rule from prose.handleis a label, not a key (776a0a9) — both fields opened with "Short unique identifier", promising a guarantee nothing provides. Uniqueness is unenforced deliberately, not pending: the petname model makes global uniqueness incoherent, per-stack uniqueness adds nothing over thedidthat already identifies the profile, and nothing in the library resolves an entity or group by handle at all. The wording states what the field is rather than qualifying a misleading phrase.putAttachment()returns the record (b6bbd26) — the uploader got back only afileId, so the record they had just created was the one thing they couldn't address, andfilename(its only mutable field) needs an id to set. Every path already had the record in hand and was discarding it.Closes #147.
Spec
docs/spec/adapters.md§ Lifecycle — new. Flush-then-release ordering, idempotentclose(),StackClosedErrorand why it's outside the taxonomy, and what stays readable after close.docs/spec/wire-format.md§ Version negotiation — new. The major/minor rule, stated normatively for third-party servers.docs/spec/wire-format.md§ Upload — records that both SDK paths return the same record this endpoint returns.docs/spec/identity.md§ Entity and § Group — why handle uniqueness is unenforced, and that handle lookup rests oncontentFieldQuery.docs/spec/attachments.md— newputAttachment()signature; the query-for-your-own-metadata recipe is gone, with the remaining query example reframed for reading someone else's metadata.Wire behavior additionally pinned by two new
discoveryFixturesin@haverstack/conformance-fixtures, consumed by theadapter-apiconformance suite.Verification
All five, from a clean build (
rm -rf packages/*/distbeforepnpm run build):New coverage: flush-before-close ordering (not merely occurrence — a reversed implementation would pass an occurrence check); a failing flush still closes and propagates; double close reaching the adapter once; reads, writes, uploads and
flush()all refusing after close while the identity getters keep working; a scoped view taken before close writing no bytes after it; the six negotiation cases; the returned record'siddriving a follow-upupdate()on both the atomic and fallback paths.Notes for reviewers
close()callsadapter.flush?.()directly rather than its ownflush(). Onceflush()gained the closed-guard, routing teardown through it would makeclose()throw on the flag it had just set. The flag is still set up front on purpose: if a flush failure left the stack half-open, a retriedclose()would hit an already-closed adapter.The identity getters (
ownerEntityId,timezone,features) stay readable after close — they return values cached at open and touch no storage. That's a judgment call, documented in the Lifecycle section and pinned by a test; easy to flip if you'd rather they throw.ScopedStackneeded exactly one guard.putAttachmentis its only path that reaches the adapter without going throughStackfirst, so without it a closed stack would still write bytes before the delegatedcreate()refused. Everything else inherits the guard by delegation.Item 4 landed smaller than the issue proposed. The issue suggested "uniqueness by convention, not enforced" — but "convention" still implies app authors ought to maintain it, and appending a caveat leaves the misleading phrase in place. Dropping "unique" and stating the reasoning in the spec closes the question instead of leaving it ajar. I looked for a case for enforcement and couldn't find one: no code path anywhere reads
handleas a key.Breaking change, item 5:
Stack.putAttachment(),ScopedStack.putAttachment()andStackClient.putAttachment()now returnStackRecord & { content: AttachmentContent }instead of thefileIdstring. Callers wanting the id readcontent.fileId. The adapter-levelStackBlobAdapter.putAttachment()is unchanged — still the bytes-only primitive returning aFileId.Generated by Claude Code