refactor: reshape the Pipefy client into an engine/session split#348
Conversation
6e82ec5 to
799b0e3
Compare
b0e4b80 to
69c3055
Compare
fb1aca9 to
90f20cd
Compare
adriannoes
left a comment
There was a problem hiding this comment.
Correction to my review above
I removed the inline thread on runtime.py. That was stacking coordination, not feedback on the code at that line. Sorry for the noise.
Below is the tightened take. The only code item I am blocking on before merge is request identity (same as #351).
Summary
Solid engine/session refactor. PipefyEngine with shared endpoints and schema cache, and cheap per-request PipefyClient sessions via AuthenticatedExecutor, is the right shape. Back-compat PipefyClient(settings, auth=…) for CLI/direct SDK is preserved.
Checked out refactor/pipefy-engine-session @ 69c3055b: ruff clean; SDK+MCP 2358 passed; CLI 319 passed.
Required before merge
Request identity under stateful HTTP (inherited from #351)
The engine/session split does not change where identity comes from. RequestScopedIdentity.resolve() still does StaticBearerAuth(require_request_bearer()), and require_request_bearer() calls get_access_token(). That runs when get_pipefy_client opens a session, inside the session's long-lived server task. Under stateful streamable HTTP, that captures the initializer's contextvar snapshot, not the current caller's bearer.
Whatever fix lands for #351 (read the token from the per-message request context inside resolve(), or stateless_http=True) needs to land here too. resolve() is the seam this PR advertises for a future OBO transform.
Stacking (coordination, not a code defect on this diff)
This branch was cut before feat/302-request-scoped-identity was force-pushed. When you rebase onto the current #302 head, runtime.py will need a manual merge (for_profile from the base + engine/session from here). That is merge hygiene, not something wrong with McpRuntime as written on this branch.
Also noted (non-blocking)
- With
PIPEFY_GQL_REUSE_FETCHED_GRAPHQL_SCHEMA=true, first-caller introspection runs under the shared endpoint lock. Flag defaults off; worth a docstring if you lean on the flag. - Runtime tests no longer assert
surface="mcp"onPipefyEngine.build. - Schema-cache reuse test uses the same token twice; distinct tokens would exercise cross-identity reuse.
test_internal_api_executor.pystill namesHttpxGraphQLExecutor.
Loopback guard copy ("hosted on-behalf-of profile lands") and stored-session startup warmup are pre-existing base concerns, not regressions in #348's delta.
Verification
uv sync --locked --all-extras --dev
uv run ruff check .
uv run pytest packages/sdk/tests packages/mcp/tests -m "not integration" -q
uv run pytest packages/cli/tests -m "not integration" -q7a097be to
6c18dbe
Compare
adriannoes
left a comment
There was a problem hiding this comment.
Re-checked after bdc637fa and 6c18dbe7, the engine/session refactor looks good, and the identity fix addresses my earlier blocker.
PipefyEnginewith shared endpoints and schema cache, plus per-requestPipefyClientsessions viaAuthenticatedExecutor, is the right shape.- Back-compat
PipefyClient(settings, auth=…)is preserved. - Checked out
refactor/pipefy-engine-session@6c18dbe7: ruff clean; SDK+MCP 2375 passed.
6c18dbe to
7a11a88
Compare
… a frozen contextvar The engine/session split snapshotted the caller's bearer by calling require_request_bearer(), which read the SDK's get_access_token() (auth_context_var). That snapshot happens in the tool body via get_pipefy_client -> session_for_request -> RequestScopedIdentity.resolve, which runs in the long-lived per-session server task under stateful Streamable HTTP. That task's auth_context_var is frozen at the session's initialize bearer, so every caller's session replayed the session initializer's token. Thread the per-message request through instead: get_pipefy_client reads ctx.request_context.request (the message's own validated request, set fresh per JSON-RPC message) and passes it to session_for_request, which hands it to resolve. require_request_bearer(request) now reads request.user.access_token off that explicit argument and never touches auth_context_var, so identity tracks the current caller by construction. StartupIdentity ignores the request and returns its one startup credential. Tests drive session_for_request/require_request_bearer with a request carrying the validated user rather than setting auth_context_var.
…elpers Type the request threaded through the identity seam as Request | None end to end (require_request_bearer, AuthSource.resolve, session_for_request) instead of object | None read via getattr. Guard None explicitly and read the token behind an isinstance(user, AuthenticatedUser) check, so an upstream rename of the request's user/access_token shape fails loudly rather than collapsing to the unauthenticated branch and masking a wiring bug as an auth failure. Lift the duplicated authenticated_user/request_with_user test helpers into the shared _rs_fixtures module so the runtime and request-identity suites cannot drift, and tighten the session_for_request stubs to the method's real one-argument arity.
7a11a88 to
74eb58f
Compare
adriannoes
left a comment
There was a problem hiding this comment.
LGTM, well done @gbrlcustodio 👍🏻
There was a problem hiding this comment.
Summary
Re-checked refactor/pipefy-engine-session @ 74eb58f after the rebase onto dev and the identity follow-ups. The engine/session split looks solid, and the earlier blocker is fixed: get_pipefy_client threads ctx.request_context.request into session_for_request, and require_request_bearer(request) reads the validated AuthenticatedUser token instead of the frozen auth_context_var. I ran the full non-integration SDK+MCP and CLI suites locally (2375 + 319 passed); CI is green.
What worked well
PipefyEnginewith shared endpoints/schema cache and per-requestPipefyClientsessions viaAuthenticatedExecutoris the right shape.- Cross-caller isolation is tested (
alice/bobbearers, shared endpoint object). - Back-compat
PipefyClient(settings, auth=…)preserved for CLI/direct SDK. - Typed
Request | Noneseam withisinstance(AuthenticatedUser)fail-loud guard.
…-session # Conflicts: # packages/sdk/src/pipefy_sdk/graphql_executor.py
adriannoes
left a comment
There was a problem hiding this comment.
Re-checked @ 1d9d5e71 after the dev merge. LGTM.
The engine/session split is solid. The identity fix (f05a075d) correctly threads ctx.request_context.request through to require_request_bearer(request), avoiding the frozen auth_context_var under stateful HTTP. McpRuntime.for_profile with remote fail-fast and TestForProfile look good.
The dev merge integrated execute_allow_partial into GraphQLEndpoint cleanly. CI green; local run: 2411 SDK+MCP + 320 CLI passed.
Minor non-blocking notes: no runtime-level assert for surface="mcp" (endpoint telemetry test exists); loopback guard error text still says "on-behalf-of profile lands" while the comment above is updated.
Why
One shared
PipefyClientcarries aRequestContextBearerAuththat reads the caller's validated bearer from a contextvar at outbound-call time. Identity is ambient rather than an explicit value, which has three costs:initialize, so a shared client can serve later callers with the first caller's bearer.This branch adopts the standard engine/session split (SQLAlchemy Engine/Session, httpx client vs request): a process-scoped, auth-agnostic core separated from a cheap per-request session bound to an identity, with the caller's request threaded explicitly rather than read from a contextvar. Isolation becomes a call-signature fact, the schema cache stays shared across identities, and the future transform is a one-line change at the composition boundary.
What
SDK:
PipefyClientsplit into an engine and a per-request sessionHttpxGraphQLExecutorsplits into:GraphQLEndpoint: the shared, auth-less core (URL, telemetry headers, error formatting, schema cache and lock).execute(query, variables, *, auth)takes the credential per call and opens a fresh transport, so one endpoint and its one schema cache serve every identity.AuthenticatedExecutor(frozen dataclass): aGraphQLEndpointbound to one identity'sauth, implementing the unchangedGraphQLExecutorprotocol. Cheap to build one per request.PipefyEngineholds the sharedPipefyEndpointsand exposessession(auth) -> PipefyClient.PipefyClientbecomes the session and gainsfrom_executors(...). The back-compatPipefyClient(settings, *, auth, surface)constructor andbuild_executorsstay and converge on the same wiring, so the CLI and direct SDK use are untouched.MCP: source request identity via the engine/session runtime
McpRuntimeowns onePipefyEngine(built auth-agnostic at construction, no network I/O) and openssession_for_request(request)per call.RequestScopedIdentity.resolve(request)snapshots the request's validated bearer into aStaticBearerAuth.get_pipefy_clientreadsctx.request_context.requestand threads it through, so the bearer comes off the message's own validated request rather than the frozen contextvar.RequestContextBearerAuthis deleted;AuthStrategyis renamedAuthSource.McpRuntime.for_profile. Theremoteprofile selectsRequestScopedIdentityand fails fast without a configured resource server; every other profile resolves the one startup credential and fails fast when none is configured.A future auth transform is now a change to
resolve(request)alone.Out of scope
resolve(request)seam is left clean).