Skip to content

feat(tools): export the llm session resume payload type and schema from the root [SAP-3184] - #821

Open
gwitwer wants to merge 4 commits into
mainfrom
feat/SAP-3184
Open

feat(tools): export the llm session resume payload type and schema from the root [SAP-3184]#821
gwitwer wants to merge 4 commits into
mainfrom
feat/SAP-3184

Conversation

@gwitwer

@gwitwer gwitwer commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Problem and motivation

The two llm deferred lanes are asymmetric at the @sapiom/tools boundary. The async lane exports everything an author needs to type and validate a resumed step's input from the package root (LlmRouteResultPayload, llmRouteResultSchema, LlmRouteResultSchemaError). The session lane exports none of it: a step paused on llm.createSession(...) and resumed on LLM_SESSION_READY_SIGNAL has no root-exported type for its input and no validator, so anyone using it writes their own types and their own validation (or reaches for the ./llm subpath).

Summary and scope

Additive only. From packages/tools/src/llm/index.ts:

  • LlmSessionReadyPayload — the input a step resumed from pauseUntilSignal(sessionHandle, …) receives. An LlmSession narrowed to the two shapes the engine's resume forwarder delivers: state: "ready" (hand it to callSession; baseUrls carries the session-scoped URLs when reported), or state: "failed" with a non-empty error carrying the gateway's structured reason (deadline_exhausted, grant_mint_failed, session_ready_failed, session_unsupported). The signal fires once, when the session leaves pending, and the forwarder folds any non-ready outcome into failed, so no other state arrives on it.
  • llmSessionReadySchema / LlmSessionReadySchemaError — runtime validator + typed error, same shape of API as llmRouteResultSchema / LlmRouteResultSchemaError. baseUrls is optional (as on LlmSession) and validated as a complete pair only when present.

From packages/tools/src/index.ts (the barrel), new root exports:

  • LlmSessionReadyPayload, LlmSession, LlmSessionState (types), llmSessionReadySchema, LlmSessionReadySchemaError
  • Nice-to-haves from the ticket, same class of gap: RoutingLabel, ModelLabel, LlmDisclosureResult (types) and readDisclosure.

scripts/provider-neutral-copy-check.mjs gains one narrow mask for the const tuple that names the two base-URL keys the schema validates.

Out of scope: no server or gateway change; no change to any existing export's name or shape.

Related work

Related issue or discussion: SAP-3184 (part of epic SAP-3183).

Validation

pnpm --filter @sapiom/tools typecheck                                  — clean
pnpm --filter @sapiom/tools lint                                       — clean
pnpm --filter @sapiom/tools build                                      — ok (cjs + esm)
pnpm provider-copy:check                                               — passed (129 audited files)
pnpm examples:check:test                                               — 165 tests, 0 failures
npx jest --maxWorkers=1 src/llm/sessions.spec.ts src/smoke.spec.ts    — 2 suites, 21 tests passed

Tests and documentation

  • llm/sessions.spec.ts: new llmSessionReadySchema block — accepts a well-formed ready payload with and without baseUrls (and the union narrows on state), accepts a failed payload for each of the four gateway terminal reasons, rejects malformed payloads with LlmSessionReadySchemaError (non-object, missing sessionId, non-terminal state, partial or non-object baseUrls, failed without a reason, mistyped optional fields).
  • smoke.spec.ts: asserts both lanes' resume contracts (signal + payload type + schema + error) import from the root symmetrically, plus readDisclosure, RoutingLabel, ModelLabel.
  • JSDoc on the new type/schema mirrors LlmRouteResultPayload's and cross-links the two.

Compatibility and release impact

  • Breaking or externally visible changes: None. New exports only.
  • Changeset: Added (@sapiom/tools: minor).

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability. I
    will follow the
    Security Policy for
    private reporting.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

Written with Claude Code. It derived the payload shape from what the engine's resume forwarder delivers to a resumed step, wrote the schema, tests and changeset, and verified with the commands above.

Checklist

  • I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
  • This pull request addresses one focused problem and contains no unrelated cleanup.
  • I added or updated tests, or explained above why tests are not applicable.
  • I ran the relevant build, typecheck, lint, and test commands, or explained
    any N/A checks above.
  • I updated documentation for user-facing changes, or marked it N/A above.
  • I added a Changeset for a published-package change, or explained why it is not applicable.
  • I can explain and maintain every submitted change, including any AI-assisted work.

🤖 Generated with Claude Code

https://claude.ai/code/session_01W5yTA5mh9qL5P6e3oBc6GH

gwitwer and others added 2 commits September 5, 2026 03:00
…om the root

The two llm deferred lanes were asymmetric at the package boundary: the async
lane exported LlmRouteResultPayload + llmRouteResultSchema +
LlmRouteResultSchemaError from `@sapiom/tools`, while a step paused on
llm.createSession had nothing to type or validate its resumed input with, so
authors hand-rolled both.

Add LlmSessionReadyPayload (an LlmSession narrowed to the two terminal shapes
the engine's resume forwarder delivers: `ready` with session-scoped baseUrls,
or `failed` with the gateway's structured reason), plus llmSessionReadySchema
and LlmSessionReadySchemaError, mirroring the route-result trio. Also re-export
LlmSession, LlmSessionState, RoutingLabel, ModelLabel, readDisclosure and the
disclosure types from the root — same class of gap, no behavior change.

Purely additive; no existing export changes shape or name.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W5yTA5mh9qL5P6e3oBc6GH
The provider-neutral copy guard audits packages/tools/src/llm/index.ts and
flagged the new schema's bare `baseUrls.anthropic` / `baseUrls.openai` property
accesses and error strings. Name the two wire-shape keys once as a const tuple
and iterate it, and unmask exactly that tuple in the guard — the same narrow,
one-usage-at-a-time approach the file's other wire-shape identifiers use. No
behavior change: the schema still requires both base URLs on a ready session
and produces the same error messages.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W5yTA5mh9qL5P6e3oBc6GH
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #821 feat(tools): export the llm session resume payload type and schema from the root

Additive, tested, correctly changeset'd as minor. Shipped prose (changeset, JSDoc, barrel comments) is provider-neutral and names no company but Sapiom; SESSION_BASE_URL_SHAPES is correctly masked in scripts/provider-neutral-copy-check.mjs. Three findings.

🔒 CONFIDENTIALITY — PR body names private-repo source files

The PR description and the AI-assistance note point at internal service internals by filename: llm-route-callback.controller.ts "in the Sapiom monorepo" and the gateway's async_worker.py / toLlmSessionPayload mapper. This PR is on a public repo; that is internal-only context on a public surface, and it is more than the change itself reveals.

The shipped copy is clean — the JSDoc and changeset both say "the engine's resume forwarder", which is the right generic role. Apply the same wording to the PR body (gh pr edit 821). Nothing needs re-publishing; a PR body is editable, unlike a changeset.

Validator asserts backend behavior stricter than this repo can consume or verify

packages/tools/src/llm/index.ts:688llmSessionReadySchema.parse throws on payloads the SDK would handle fine:

  • baseUrls is hard-required on ready (both keys, both strings), but nothing in this package reads it. callSession (llm/index.ts:847) takes LlmSessionHandle | LlmSession | string, pulls only sessionId, and builds the URL from DEFAULT_BASE_URL. So a ready payload carrying sessionId but no base_urls — entirely usable via callSession — is rejected as "malformed". The JSDoc's own advice ("hand the payload to callSession") works without baseUrls.
  • Only ready/failed are accepted, while this lane's own handle.wait() treats ready | active | expired | exhausted | failed as settled (SESSION_SETTLED, llm/index.ts:583). A webhook that lands after the session moved to active, or a session that expired before resume, arrives at the resumed step as a thrown LlmSessionReadySchemaError rather than a state the author can branch on. The async lane avoids this because toPayload (llm/index.ts:303) normalizes expired/lost into status: "failed" inside this package; the session lane's equivalent mapping lives in a service this repo cannot test against, so nothing here pins the shape.

Failure scenario: engine forwards {sessionId, state: "ready"} (or state: "active"); llmSessionReadySchema.parse(input) throws at the resume boundary and the step fails, though callSession(input, …) would have succeeded. There is no safeParse, so a consumer who follows the documented pattern has no non-throwing path.

Pick one: (a) make baseUrls optional in the ready branch and validate its keys only when present, and fold the other settled states into the union or into a normalized failed; or (b) keep the strictness and cite the forwarder guarantee in the JSDoc plus add safeParse. (a) is cheaper and matches what the package actually consumes.

Root barrel picks up exports the PR calls "nice-to-haves"

packages/tools/src/index.ts:110-118. Root exports are permanent under semver — removal needs a major — so the incidental ones deserve the same scrutiny as the load-bearing ones:

  • readDisclosure is the only unprefixed, capability-agnostic value at this level; every sibling is prefixed (llmRouteResultSchema, modelRunResultSchema, agentResultSchema, toResumePayload excepted). It is already reachable three ways — llm.readDisclosure via the namespace at index.ts:82, the ./llm subpath, and sapiom.llm.readDisclosure (client.ts:577) — so the root export buys nothing and claims a name a future capability's disclosure reader will want.
  • LlmDisclosure (llm/index.ts:359) is the raw snake_case wire interface. Exporting it at the root commits the package to the wire shape; LlmDisclosureResult is the camelCase view consumers should hold.

LlmSessionReadyPayload, llmSessionReadySchema, LlmSessionReadySchemaError, LlmSession, LlmSessionState are load-bearing for the stated problem — keep those. Drop the bare readDisclosure and LlmDisclosure, or say in the changeset why the root needs them; RoutingLabel / ModelLabel are fine (they type model / label args on root-reachable calls).


Verdict: Approve after the validator strictness is resolved (relax or justify) and the PR body is de-internalized; the export trim is a judgment call the author can decline with a sentence.

…; trim root exports

Review round 1: `llmSessionReadySchema` required `baseUrls` on a ready payload,
which is stricter than anything in this package consumes — `callSession` needs
only `sessionId`. Make it optional on the payload (as it is on `LlmSession`) and
validate the pair only when present. The state union stays `ready | failed`:
the signal fires once when the session leaves `pending`, and the engine folds
any non-ready outcome into `failed` with the outcome as the reason, so no other
state arrives on it.

Also drop the raw wire-shape `LlmDisclosure` from the root barrel (it stays on
the `llm` namespace / subpath); the camelCase `LlmDisclosureResult` and
`readDisclosure` — which the ticket asks for — remain.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W5yTA5mh9qL5P6e3oBc6GH
@gwitwer

gwitwer commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — addressed in 14e6bb3 and the edited PR body.

Confidentiality. Fixed. The PR body and AI-assistance note now use the same generic wording as the shipped copy ("the engine's resume forwarder") and name no internal files.

Validator strictness. Partly agreed, partly declined:

  • baseUrls — agreed. It is now optional on LlmSessionReadyPayload (as it is on LlmSession) and validated as a complete, well-typed pair only when present. callSession needs only sessionId, so a ready payload without base URLs parses. Test added.
  • State union — declined. LLM_SESSION_READY_SIGNAL fires exactly once, when the session leaves pending; the forwarder maps the webhook body, not a later GET, and folds any non-ready outcome into state: "failed" with the outcome as the reason — the same normalisation the async lane does in toPayload. So ready | failed is the complete set that arrives on this signal, and active/expired/exhausted are states you get from getSession/wait(), not from a resume. Accepting them here would advertise shapes that never arrive. I have put that guarantee in the JSDoc. safeParse is not added: it would diverge from llmRouteResultSchema, which this ticket is explicitly mirroring.

Root exports. LlmDisclosure (raw wire shape) dropped from the root; it stays on the llm namespace and subpath. readDisclosure stays: the ticket lists it by name as a root re-export, and LlmDisclosureResult stays as its return type.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #821

Delta since ebd9337: one commit, 14e6bb3 (refactor(tools): make baseUrls optional…; trim root exports).

Earlier findings — status

  • 🔒 CONFIDENTIALITY (PR body named private-repo source files)fixed. The body and the AI-assistance note now say "the engine's resume forwarder" / "the gateway's structured reason"; no internal filenames remain.
  • Validator stricter than the package can consume — baseUrlsfixed. baseUrls is now optional and validated as a complete pair only when present (llm/index.ts:712), matching callSession, which reads only sessionId. Tests cover the ready-without-baseUrls and baseUrls: null cases.
  • Root barrel trimpartly taken. LlmDisclosure (the snake_case wire interface) is gone from the root and the barrel comment says why. Bare readDisclosure stays with no stated reason; that is the author's call to make and not a blocker.

Still open

  • No non-throwing parse path for an unexpected state. llmSessionReadySchema.parse still rejects every state but ready/failed (llm/index.ts:701), and the justification is now a JSDoc assertion about a forwarder in a service this repo cannot test against (llm/index.ts:659-662). That is the "cite the guarantee" half of the earlier option (b) without the safeParse half: if the guarantee ever slips, the resumed step throws at the boundary with nothing to branch on. Adding safeParse is a one-liner and keeps the strict default.

New

  • LlmSession.baseUrls JSDoc now contradicts the payload it types. llm/index.ts:635 still reads "Present from READY on: …", while the same commit's payload JSDoc and changeset say baseUrls arrives "when the gateway reported them". Both ship to npm in the same release; a consumer reading the interface sees a guarantee the validator deliberately no longer enforces. Reword line 635.

Nothing the earlier round got wrong: callSession (llm/index.ts:852-864) does ignore baseUrls, as claimed. The provider-neutral mask still matches the const tuple it was added for.

Verdict: Approve. Fix the LlmSession.baseUrls doc line; safeParse is worth adding but can follow.

…ayload contract

Review round 2: the field doc still promised "present from READY on" while
the session resume schema deliberately treats it as optional. Say when the
gateway reports it and that callSession does not need it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W5yTA5mh9qL5P6e3oBc6GH
@gwitwer

gwitwer commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — addressed in 1083124.

LlmSession.baseUrls JSDoc — fixed. It now says the URLs are present once the session is READY and the gateway reports them, and that callSession does not need them. That matches the payload JSDoc, the changeset, and what the validator enforces.

safeParse — declined for this PR, deliberately. The ticket's brief is to mirror llmRouteResultSchema / LlmRouteResultSchemaError so the two lanes are symmetric at the root; giving only the session schema a non-throwing path would reintroduce an asymmetry between the lanes, and adding it to both is a separate, cross-lane change. Agreed it is worth doing — as a follow-up covering both schemas together.

readDisclosure — stays for the reason given in round 1: the ticket lists it by name as a root re-export.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant