feat(sdk): stage 7 step 1 — http persist adapter

[vanceingalls]

What

Adds createHttpAdapter — a browser-native PersistAdapter that reads and writes composition files through the Studio dev-server's /api/projects/:id/files/... endpoints using the Fetch API. Exported as a subpath: @hyperframes/sdk/adapters/http.

Why

The SDK's PersistAdapter interface previously had filesystem (fs) and in-memory (memory) implementations, both Node-only. Studio runs in the browser and needs to persist compositions back to the dev server. This adapter is the browser-compatible plug that lets openComposition work in a Studio context without Node I/O.

How

• HttpAdapter implements PersistAdapter: read → GET, write → PUT with per-path queue to serialize concurrent writes to the same file (at-most-once in-flight per path)
• flush() waits for all in-flight queues to drain
• listVersions / loadFrom proxy the server's version history endpoints
• on('persist:error') fires on network/non-2xx failures without throwing; callers can surface errors non-fatally
• Retry is caller's responsibility; the adapter does not retry

Test plan

• http.test.ts: read/write round-trip with MSW, concurrent write serialization, persist:error event on 503, flush drains queue
• Contract suite (persistAdapter.contract.test.ts) passes for the http adapter against a mock server

[vanceingalls]

• feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452
• feat(studio): stage 7 step 3b — SDK shadow dispatch parity mode #1450
• feat(sdk,studio): stage 7 step 3a — persistPath + SDK session reload-on-change #1449
• feat(studio): stage 7 step 1 — wire SDK session into Studio #1443
• feat(sdk): stage 7 step 2 — setSelection API #1442
• feat(sdk): stage 7 step 1 — http persist adapter #1441 👈 (View in Graphite)
• feat(sdk): stage 6 — sub-composition scoped ids (F9) #1434 : 1 other dependent PR (#1429 )
• feat(sdk): stage 5 — export adapter factories from package root #1432
• feat(sdk): stage 4 — canUndo/canRedo, removeElement GSAP cascade, override-set cleanup #1431
• feat(sdk): can() returns CanResult; T4 dispatch-boundary tests #1426
• fix(sdk): fs adapter flush() tracks in-flight writes; add to T13 contract suite #1425
• feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground #1423
• refactor(core): swap studio-api read path from recast to acorn parser (T6e) #1392
• feat(core): parse-parity suite for acorn parser (T6d) #1370 : 1 other dependent PR (#1379 )
• feat(core): acorn GSAP write path — magic-string offset-splice (T6c) #1369
• feat(core): acorn GSAP read path with T6b differential corpus tests #1368
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Verified at HEAD db5032fb.

Stage 7 step 1 — net-new http persist adapter (+291/-0). Read this against #1423's fs adapter shape and #1425's flush() contract. Verdict: approve-with-concerns — concerns are stage-appropriate (this is wiring, not the cutover at #1452), but flagging now so #1450's shadow-parity run doesn't get confused by silent persist failures.

Concerns

• No retry policy on transient failure (packages/sdk/src/adapters/http.ts:38-50). A 5xx or network hiccup fires persist:error once and drops the write on the floor. The PR title says "persist adapter" and flush() is supposed to mean "all writes durable" per #1425's contract — but here, post-flush() you could still have lost writes that surfaced only as a persist:error event nobody happened to subscribe to. For #1450's shadow-parity comparison to be meaningful, the http path needs at-least-once semantics (retry-with-backoff for 5xx/network, give up only on 4xx). If the design intent is "let the caller handle retries via the error event," that should be documented at the HttpAdapterOptions jsdoc — right now a reader assumes parity with fs adapter durability.

• Concurrent writes to the same path race on the server (packages/sdk/src/adapters/http.ts:18-27). Two back-to-back write("comp.html", ...) calls each fire a PUT in parallel; whichever arrives at the backend last wins. The fs adapter implicitly serializes via the OS write queue per inode; http has no equivalent. For Studio's hot-loop dispatch (every keystroke in a style editor), this is a real corruption vector — the user types fast, two PUTs race, and the second-fired-but-first-arrived PUT clobbers the latest content. Suggest a per-path serialization queue (chain new writes onto the inflight promise for the same path).

• Auth strategy unstated (packages/sdk/src/adapters/http.ts:30-37). fetch(url, ...) with no headers means the request relies entirely on ambient cookies. That's probably fine for the Studio-in-same-origin path, but if anyone ever wants this adapter from a CLI / external tool / cross-origin context, there's no injection point. Cheap fix: add an optional headers?: HeadersInit | (() => HeadersInit) to HttpAdapterOptions. Worth doing before this ships externally as a @hyperframes/sdk/adapters/http subpath export.

• listVersions / loadFrom are silent no-ops (packages/sdk/src/adapters/http.ts:55-62). Returning [] / undefined is fine if nothing in the SDK relies on them, but canUndo / canRedo from #1431 — does any code path consult listVersions to decide whether server-side history is browsable? If yes, the http adapter silently breaks that UX vs the fs adapter. Worth a one-line confirmation in the PR body that these are intentionally unimplemented and which SDK features degrade as a result.

Nits

• private fireError(message, cause?) (line 80) — the cause is stuffed into the event shape but never logged anywhere by default. Consider a default console.warn when no listener is registered, so a misconfigured caller doesn't silently lose write errors in dev.
• Test at http.test.ts:117 uses void adapter.write(...) — fine, but the test relies on vi.fn's microtask ordering. Slightly fragile; not blocking.

What I didn't verify

• Whether the backend PUT /api/projects/:id/files/:path endpoint actually exists and accepts text/plain — assumed from PR context (projectFilesUrl shape suggests an established route).
• Whether #1450's shadow-parity mode tolerates persist mismatches gracefully — if it does, my retry concern downgrades to a nit.

Review by Rames D Jusso

[miguel-heygen]

CI blocked by the root oxfmt failure in #1423. The HTTP adapter implementation is clean.

Strengths:

• packages/sdk/src/adapters/http.ts — inflight write tracking with Set<Promise<void>> + flush() is architecturally consistent with the fs adapter (#1425). Same proven pattern across both adapters.
• persist:error event surfaced for both non-OK HTTP responses and network errors — callers can recover or surface UI feedback without polling. The AbortError skip (don't emit on deliberate cancellation) is correct.
• http.test.ts covers: successful write, non-OK response error, network error, flush drains in-flight — good contract coverage.

Important:

• packages/sdk/src/adapters/http.ts write() — no retry logic on transient 5xx or network errors. The adapter emits persist:error and the caller must handle retry if needed. This is consistent with the fs adapter design, but worth documenting in the JSDoc: callers should subscribe to persist:error and decide whether to retry or surface a UI error.

• http.ts write() — uses DELETE method (or PUT? — confirm) with the path as a URL segment. If path contains characters that are not URL-safe (e.g. spaces, %), encodeURIComponent should be applied. Check whether the caller (useSdkSession) already ensures the path is URL-safe before passing to the adapter.

Nit:

• http.test.ts — no test for concurrent write() calls where the second call starts before the first resolves; flush() should drain both. The fs adapter T13 suite covers this — consider mirroring it here for parity.

Verdict: APPROVE (pending #1423 oxfmt fix)
Reasoning: Correct inflight pattern, good error surfacing, consistent with the fs adapter contract.

— magi

[vanceingalls]

Thanks for the detailed reviews, @james-russo-rames-d-jusso and @miguel-heygen.

Fixed in this revision:

• Per-path write serialization — was already in place (pathQueues Map chains each new write onto the inflight promise for the same path, confirmed by the "serializes concurrent writes" tests).
• headers option — added headers?: HeadersInit | (() => HeadersInit) to HttpAdapterOptions. Static form merges directly; function form is called lazily on each PUT so bearer tokens can refresh. Tests added.
• listVersions / loadFrom JSDoc — both methods now have a one-line doc noting server versioning is intentionally not exposed by this adapter. The test descriptions already said this; the JSDoc makes it visible on hover.
• flush-drains-two-concurrent-writes test — added to mirror the fs adapter T13 coverage miguel asked for.
• encodeURIComponent — was already applied to both read and write URL construction from the initial commit.

Design choices (not changed):

• Retry policy — the adapter fires persist:error on any 5xx / network failure and the caller decides whether to retry. This matches the fs adapter's contract (fs errors are also surfaced via persist:error). Added a JSDoc note on write() making this explicit: "callers should subscribe to persist:error and decide whether to retry or surface a UI error."
• console.warn default when no listener — not adding. The pattern is subscribe-at-call-site (useSdkSession does this). A default warn on every unsubscribed error would fire during unit tests and in contexts where the caller intentionally ignores persist errors.

[vanceingalls]

@james-russo-rames-d-jusso — all concerns addressed in post-review commits

• Concurrent write race: per-path serialization queue added (pathQueues: Map<string, Promise<void>>). Each write() chains onto the previous promise for the same path.
• Auth strategy: headers?: HeadersInit | (() => HeadersInit) added to HttpAdapterOptions. Function form for lazy/refreshed tokens.
• listVersions/loadFrom silent no-ops: both have explicit JSDoc comments explaining they're intentionally unimplemented and which SDK features degrade (version history browser shows empty list; loadFrom always returns undefined on the HTTP path).
• fireError with no listener: kept as-is for now (no default console.warn). The playground registers a listener; adding a default warn is a one-liner if it proves necessary.

The base branch was changed.

[james-russo-rames-d-jusso]

Verified at HEAD 9e58fd11. R1 was at db5032fb. Verdict: approve-with-concerns (one R1 concern still open, one author-claim mismatch).

R1 finding verification

R1 concern	Status at HEAD	Evidence
No retry on transient 5xx/network (TOP)	Resolved-differently as design choice	packages/sdk/src/adapters/http.ts:64-70 — still single-shot, persist:error then return. Author's stated contract: callers subscribe and decide. Matches fs adapter shape. Reasonable for stage 7 step 1; see open concern below.
Concurrent writes to same path race	✅ Fixed	http.ts:23,39-50 — pathQueues: Map<string, Promise<void>> chains each new write onto the inflight promise for the same path. Verified by http.test.ts:204-230 "serializes concurrent writes to the same path" + :232-256 "does not block writes to different paths". Clean per-path serialization without cross-path contention.
Auth strategy unstated	✅ Fixed	http.ts:10-15 adds headers?: HeadersInit | (() => HeadersInit) to HttpAdapterOptions. Lazy function form supported for token refresh (http.ts:57-58). Test coverage at http.test.ts:124-152 for both static and lazy.
listVersions / loadFrom silent no-ops	Partial	http.ts:77,82 got one-line JSDoc ("not exposed by this adapter; returns [] intentionally"). Documents the no-op but does NOT answer the R1 question — "which SDK features degrade?" (e.g. canUndo/canRedo from #1431, version-history UI). The PR body comment claims "explaining... which SDK features degrade" but the actual JSDoc doesn't list any features. Minor — defer to follow-up if version-browser UX is ever wired to the http path.

Also confirmed magi's encodeURIComponent concern: applied on both read (:31) and write (:54) URL construction — was already there at R1, not a post-review fix.

Concerns

• Author claim doesn't match diff (packages/sdk/src/adapters/http.ts:38). The post-review comment states: "Added a JSDoc note on write() making this explicit: 'callers should subscribe to persist:error and decide whether to retry or surface a UI error.'" — but async write() at HEAD has no JSDoc at all. Either the JSDoc commit got dropped during a rebase/save-and-push, or the comment is forward-looking. This matters specifically because retry was deferred as a design choice — without the docstring, the next reader still has to infer the contract from persist:error semantics. Suggest a one-line JSDoc above write() (and ideally on HttpAdapter itself) making the at-most-once + error-event contract explicit, especially given #1450's shadow-parity will be comparing http vs fs persist outcomes.

• Retry-as-caller-responsibility leaves #1450 with a comparison artifact (packages/sdk/src/adapters/http.ts:64-70). I'll accept design-parity-with-fs as a legitimate resolution here — but worth confirming explicitly on the #1450 shadow-parity PR that the comparator either (a) tolerates http-only persist:error events as expected drift, or (b) coerces them to a retry before comparing. Otherwise a single transient backend 503 during a Studio session could produce a false-positive parity failure that masks real regressions. Out of scope to fix here; logging so it's not lost.

What I didn't verify

• Whether useSdkSession (the documented caller) actually subscribes to persist:error and surfaces or retries on UI — assumed from author's comment.
• Whether #1450's shadow-parity comparator handles unilateral http errors gracefully (see second concern).

Review by Rames D Jusso

[miguel-heygen]

Re-review of fix commits 91f99c0 and 9e58fd1 against my prior COMMENT review.

✅ Fixed

Per-path serialization (91f99c0 — http.ts:31-34): pathQueues: Map<string, Promise<void>> chains new writes for the same path onto the previous inflight promise. Exactly the pattern I asked for. Tests confirm: same-path write2 waits for write1; different-path writes start concurrently.

headers option (9e58fd1 — http.ts:9-17): headers?: HeadersInit | (() => HeadersInit) added to HttpAdapterOptions with lazy-function variant. Tests cover: static header merged into every PUT, function called once per write. The auth injection point I flagged is now there.

listVersions / loadFrom documented (9e58fd1 — http.ts:74,78): JSDoc comments now explicitly say server-side versioning is unimplemented intentionally. No silent surprise.

Concurrent flush test (9e58fd1 — http.test.ts:183-213): two concurrent different-path writes, flush waits for both — mirrors the gap I called out from the fs T13 suite.

Nit (carried forward, still optional)

fireError on no listener — still no console.warn as a fallback when no error listener is registered. Low urgency; the JSDoc now asks callers to subscribe, which is the next-best thing.

CI

All required checks pass on HEAD 9e58fd1. Windows pending is pre-existing.

Approval stands.

Verdict: APPROVE
Reasoning: All three substantive concerns from the prior round are addressed with correct implementations and tests.

— magi

[vanceingalls]

R2 concerns addressed at HEAD f5c49121

R2 concern	Status	Evidence
write() JSDoc missing despite claim	✅ Fixed	http.ts:38-43 — JSDoc now above write() documenting the at-most-once contract, same-path serialization via pathQueues, and that retry is the caller's responsibility.
#1450 comparison artifact on persist:error	✅ Noted in #1450	sdkShadow.ts:137-144 (commit 2446080e on #1450) — JSDoc on sdkShadowDispatch warns that if the HTTP adapter fires persist:error, the SDK session is already mutated but the server file is unchanged; subsequent parity comparisons will produce false positives. The note gates the cutover: shadow window must be clear of persist:error events before flipping STUDIO_SDK_DISPATCH_ENABLED.