feat(etl-uvicorn): give plugins request-scoped settings and invocation context - #74
feat(etl-uvicorn): give plugins request-scoped settings and invocation context#74CyMule wants to merge 31 commits into
Conversation
3159a2c to
2d66570
Compare
Moves the ASGI middleware, the /metadata capability route and the request-scoped binding out of utic-invocation-settings and into this package, as unstructured_platform_plugins.invocation_settings. The split follows what the two halves actually are. The contract — which keys carry settings, how a sealed envelope is told from plaintext, what an absent field is allowed to mean — stays in the library: the absence rule is the tenant-confusion vector, and it belongs next to the crypto it governs and the threat model that describes it. Buffering a request body and registering a route do not. Two things this buys immediately: - No more duck-typing. Living in the library forced the middleware to reach into `app.router.routes` through getattr chains to avoid importing Starlette. Here fastapi is already a dependency, so route eviction and the ASGI signature are typed against the real thing. - The library goes back to cryptography + pydantic with no framework test dependencies at all; its packaging suite asserts that against the built wheel. Requires utic-invocation-settings >=0.4.0 for resolve_invocation_settings, http_status_for and the contract constants. Tests: 109 passed (84 + 25 ported transport tests), ruff clean.
A plugin consuming current_invocation_settings() builds its handler per distinct settings payload instead of once at boot, and construction typically does network work (model resolution, prechecks). This gives that pattern one home next to the accessor that creates the need: settings_cache_key digests the canonical settings JSON so secret-bearing payloads are never raw keys, and SettingsScopedCache memoizes derived state bounded by both size and age — age matters because state built from since-rotated credentials must not outlive them on a quiet pod. Stdlib-only, so the package's dependency set is unchanged.
An invocation_context with an unreadable schema_version is deployment skew between platform components; answering 422 let an upstream blame classifier pin it on the caller. Context failures now take their status from http_status_for like settings failures already did: malformed fields stay the caller's 422, version skew answers 500 with the class name only. Also documents the two capability tiers on /metadata: the unconditional strings are transport-level facts the middleware makes true for every wrapped app; invoke_with_sealed_dag_node_settings is the consumption claim and stays a per-plugin opt-in.
…ding it in status codes Status codes carry transport semantics for the immediate caller and cannot also carry business blame: a 422 for a malformed reserved field (composed by the platform) and a 422 for a customer's unreadable file are different faults wearing the same number. Failure responses now say whose fault it is explicitly: - the invoke envelope gains an optional `blame`, set to "user" only when the plugin raised the UserError family — a fault in something the customer owns. Absent means not-the-customer's: an orchestrator must never infer customer fault from the status class alone. - middleware error bodies carry the invocation-settings taxonomy `reason` code alongside `detail`, so an orchestrator can recognize a platform-composed payload failure whatever status answered the hop.
…tus spelling unstructured_platform_plugins.invocation_context holds the /invoke identity contract: InvocationContext, extract_context, dimensions, the reserved context key, the dimension fields, the supported versions, and UnsupportedContextVersionError. The context is protocol identity - no crypto, no secrets - so it ships with the plugin protocol; its errors subclass the shared InvocationSettingsError taxonomy so hosts classify context failures with the same reason/blame machinery as settings failures. http_status_for - the HTTP spelling of the library's normative blame -> status rule - lives with the middleware that emits the responses.
- Replace raw `/invoke` body buffering and replay with a FastAPI dependency that reads Starlette's cached JSON parse. - Keep `InvokeBodyLimitMiddleware` below FastAPI as a streaming byte counter, so oversized bodies are rejected without a second buffer. - Install binding through the router's public dependency list before `POST /invoke` is registered; remove private `route.dependant` mutation. - Re-enter the captured invocation binding inside async-generator response iteration, so streaming plugins see settings on the repository's locked FastAPI 0.117.1 as well as newer FastAPI releases. - Preserve the explicit `invoke_with_sealed_dag_node_settings` capability opt-in and its wrapper, generator, and CLI arguments. - Align transport tests with the shared contract: sealed settings are accepted only at `invocation_settings.dag_node_settings`; a bare envelope fails closed. ASGI middleware has to consume the raw receive channel before FastAPI can parse it, which forced the old implementation to buffer, parse, and replay the request. Route-level extraction shares the framework's body and JSON caches instead. The initial dependency version still had three correctness problems: FastAPI 0.117.1 closed yield dependencies before `StreamingResponse` iteration, installation mutated FastAPI's private dependency graph after route registration, and unconditional sealed-capability advertisement conflated transport support with handler consumption. This revision fixes all three without raising the FastAPI floor. - `install_invocation_envelope(app)` must run before `POST /invoke` is registered. It may run after unrelated routes such as `/metadata`, which preserves the hand-written plugin integration order. - The dependency is a path/method-aware no-op outside `POST /invoke`, including mixed-method routes and rooted deployments. - Malformed JSON on a declared FastAPI body model continues to use FastAPI's own validation response. - `pytest -q` on FastAPI 0.117.1 / Starlette 0.48.0 with the current utic-invocation-settings 0.4.0 branch — 153 passed. - Focused transport tests on FastAPI 0.141.1 / Starlette 1.6.0 — 38 passed. - `ruff check .` — clean. - `ruff format --check` on changed Python files — clean. - `git diff --check` — clean.
The non-streaming failure envelope derives blame from the UserError family; the async-generator error envelope omitted it, so a customer fault raised mid-stream read as unattributed.
…oyment Per the ASGI spec, scope["path"] includes any deployment root_path; the router matches on get_route_path, which strips it. Comparing the raw path to /invoke made both envelope binding and the body cap silently skip on a spec-compliant rooted server, so settings read as absent and the boot-file fallback took over. TestClient(root_path=...) does not prefix path, so the regression tests build the spec-compliant scope themselves.
…ody cap The idempotence guard returned before reading max_body_bytes, so a second install asking for a different cap was silently ignored and the caller was left believing its limit was enforced. A same-value repeat stays a no-op.
The envelope has been extracted by a route dependency, not middleware, since the body-replay design was removed.
6bbbe43 to
f8b5104
Compare
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.
Re-trigger cubic
ctrahey
left a comment
There was a problem hiding this comment.
Reviewed against the whole era-1 train — #67, #1333, and VLM #556 (merged) / #576 (open).
Three things are genuinely right and I want them on the record before the asks. The capability layering is default-deny per format — _supports_sealed_dag_node_settings in #1333 dispatches on the document's own format before picking a capability — which is what makes it safe for every wrapped plugin to start advertising transport capabilities by construction. I chased that as a possible fleet-wide break on a dependency bump and it isn't one; worth saying so in the description, since the safety lives in a different repo. The invocation_settings: null guard does not collapse null into absence, which is precisely the vulnerability #67's README warns about. And re-entering the captured binding inside _stream_response is the right fix for the yield-dependency cleanup ordering.
I'm requesting changes on one architectural point, and holding the merge on it. The rest are specific.
1. Wire types ratify in the schema registry — they are not shared-lib imports (blocking)
InvocationContext, DIMENSION_FIELDS, RESERVED_CONTEXT_KEY and the new blame value are cross-service wire contracts. Our direction is that projects refer to a type by $id in schemas-experimental (schemas.u10d.dev) and code-generate locally, rather than import a shared library to obtain the type.
That is not hypothetical for this family: ratified-types/invocation-settings/field-envelope/v1.json is already ratified and is already cited as structural authority by envelope-contract-v2.md, alongside oauth/ and secrets/. The precedent is established; these two types just haven't been added yet.
invocation_context.pyshould become a ratifiedinvocation-context/v1with locally generated bindings here and in the controller. As written, #1333 has the controller importing it fromunstructured_platform_plugins— so the controller takes fastapi, uvicorn,unstructured-ingestand the OTel gRPC exporters to obtain a pydantic model and a tuple of strings. VLM #576 refuses the dependency outright, which means the one plugin actually proven end-to-end on an SND cannot use the canonical identity model at all.DIMENSION_FIELDSis the load-bearing part: if two services disagree on it the same request is attributed differently depending on which one emitted the event, and that disagreement is silent.blameintroduces a third vocabulary (a bare"user") alongsideutic_invocation_settings.Blameandutic_errors.Audience, in a PR whose own argument is thatBlameis canonical. That reconciliation wants to be one ratified type, not a fourth spelling.
The registry is still marked experimental, which is to say it is cheap to change — adding these two types is not a heavy lift. I would rather absorb it now than retrofit after more consumers import from here.
I'm holding this PR until both are ratified and consumed as generated bindings. That blocks #67's publish, #1333, and VLM #576 behind it, and I'm accepting that cost deliberately: this PR is the keystone of the train and is the wrong place to land import debt.
2. Drop the FF_INVOCATION_SETTINGS rename (blocking — also in #67 and #1333)
#1333 replaces FF_INVOCATION_SETTINGS with FF_REQUIRE_INVOKE_WITH_SEALED_DAG_NODE_SETTINGS on native pods, and #67 exports the new name. VLM main reads the old one (utic_ff.feature_enabled("invocation-settings")), and #576 states the assumption explicitly: "FF_INVOCATION_SETTINGS deployment-mode semantics are preserved… the library's own FF_REQUIRE_INVOKE_WITH_SEALED_DAG_NODE_SETTINGS stays unset by the operator." #576 is dated 08-20; #1333 moved on 08-31 and falsified it.
The failure is not subtle. A native VLM pod under #1333 has no init-secrets sidecar, so no settings file; _invocation_settings_enabled() returns False, so VLM loads the boot file at startup and fails its lifespan. It never reaches the library's fail-closed path — which is exactly the outcome the operator's own comment says that variable exists to prevent.
Keep FF_INVOCATION_SETTINGS. It's the name already deployed and already read by the one native consumer. One wrinkle to sort out in #67: require_invoke_with_sealed_dag_node_settings() reads os.environ directly while VLM reads through utic_ff, so the library needs either the old name or a utic_ff dependency it doesn't currently have.
3. #1333's rollout sequence needs the VLM uptake as explicit steps
vlm_partitioner/uv.lock pins utic-invocation-settings 0.3.0, which cannot read v2 documents; v2 lands in 0.4.0. #1333 makes the operator emit v2 for every sealed node. VLM main advertises only the v1 capability, so the per-format gate correctly forwards nothing — but combined with §2 that's a pod with settings from neither channel.
The rollout doc's four steps stop at "enable native plugin versions" and never say: re-lock VLM to 0.4.0, merge #576, re-pin SEAL_NATIVE_ENABLED_PLUGINS to the rebuilt image tag. That last one is mandatory and easy to miss — _version_matches falls through to exact string equality for git-* tags, so there is no "from this sha onward". Please make those explicit steps in #1333 rather than implied ones.
4. Release gate
[tool.uv.sources] is uv-only and isn't propagated to consumers, so a published 0.1.0 hands every downstream utic-invocation-settings>=0.4.0 with nothing on PyPI to satisfy it. The release workflow fires on release publish, so this is a human gate — worth a merge-checklist item rather than prose in the description.
Inline comments below on four smaller transport items, all of which I'd like addressed.
|
|
||
| try: | ||
| parsed = await request.json() | ||
| except ValueError: |
There was a problem hiding this comment.
An unparseable body lands here as parsed = None, which resolve_invocation_settings reads as genuine absence — and on a native pod that raises SealedDagNodeSettingsRequiredError (RECIPIENT → 500).
So the clearest possible CALLER fault, a body that isn't JSON, answers 500 on exactly the pods this work targets. That inverts the taxonomy the rest of this module is careful about, and it puts a malformed request into the controller's platform-blame bucket.
Worth distinguishing "no body at all" (absence, judge it) from "body present and unparseable" (422 before resolution runs). The docstring acknowledges the path but not that the blame comes out backwards.
There was a problem hiding this comment.
Addressed: malformed non-empty JSON now returns 422; an empty body remains genuine absence.
| except Exception: | ||
| # The cut body stream can surface downstream as something other than | ||
| # ClientDisconnect; once the cap is the cause, the 413 below is the answer. | ||
| if not exceeded: |
There was a problem hiding this comment.
Once exceeded is true this swallows every downstream exception, not just the one caused by cutting the body — and logs nothing. A genuine bug that happens to coincide with an oversized request disappears into a 413 with no trace.
The behavior is right; the silence isn't. A logger.debug with type(exc).__name__ costs nothing and keeps the case diagnosable.
There was a problem hiding this comment.
Addressed: we now debug-log the downstream exception type before returning 413.
| # Bounds the /invoke request body; generous because batch invokes carry an array of file_data | ||
| # payloads. Nothing below the framework buffers, so this cap is the only guard against | ||
| # unbounded-memory requests. | ||
| MAX_INVOKE_BODY_BYTES = 64 * 1024 * 1024 |
There was a problem hiding this comment.
This cap is new, unconditional, and applies to ~34 plugins where nothing bounded body size before. Batch invokes carry an array of file_data payloads, and the comment says the bound is generous — but has it been checked against observed batch sizes in production?
If it hasn't, a routine wrapper bump could start answering 413 to large batches that work today, with no opt-in from the plugin. I'd like a number behind the 64 MiB before this ships fleet-wide.
There was a problem hiding this comment.
Agreed. The default cap is now disabled; 64 MiB is available only through explicit opt-in.
| name = "unstructured_platform_plugins" | ||
| description = "Wrapper to convert arbitrary code into a uvicorn/fastapi implementation for Unstructured Platform" | ||
| requires-python = ">=3.10" | ||
| requires-python = ">=3.11" |
There was a problem hiding this comment.
Floor bump for every plugin that consumes this wrapper. Can you confirm no in-tree plugin is still on 3.10? If any is, this pins them out of the settings work entirely rather than just deferring their cutover.
There was a problem hiding this comment.
Confirmed: every in-tree plugin requires Python 3.12+, so the 3.11 floor excludes none.
| blame = Blame.CONTENT | ||
|
|
||
|
|
||
| class InvocationContext(pydantic.BaseModel): |
There was a problem hiding this comment.
This is the model from §1 of the review. DIMENSION_FIELDS in particular is a cross-service agreement where disagreement is silent — two services picking different field names attribute the same request differently depending on which emitted the event.
It should be a ratified invocation-context/v1 in schemas-experimental with locally generated bindings, referenced by $id, rather than a type other services import from this package. The field-envelope/v1.json next to it is the pattern.
There was a problem hiding this comment.
Addressed: invocation-context/v1 is now ratified, and this PR uses a locally generated binding referenced by $id.
| return category if isinstance(category, str) else None | ||
|
|
||
|
|
||
| def blame_of(error: BaseException) -> Optional[str]: |
There was a problem hiding this comment.
Third blame vocabulary: a bare "user" here, utic_invocation_settings.Blame (CALLER/CONTENT/ROUTING/RECIPIENT) in the module below, and utic_errors.Audience in the controller.
This PR's own argument is that Blame is the canonical taxonomy, so adding a fourth spelling in the same diff undercuts it. Per §1, this wants to be one ratified type consumed by codegen — the reconciliation is worth doing once rather than accreting a vocabulary per hop.
There was a problem hiding this comment.
Addressed: ErrorAudience is now ratified and generated locally, with plugin_error.audience as the canonical wire location.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="unstructured_platform_plugins/etl_uvicorn/api_generator.py">
<violation number="1" location="unstructured_platform_plugins/etl_uvicorn/api_generator.py:62">
P2: The ratified audience contract requires `plugin_error.audience`, but this model marks it nullable. Make the field required so OpenAPI and runtime validation cannot advertise or produce incomplete plugin-error metadata.</violation>
</file>
<file name="scripts/generate_invocation_contracts.py">
<violation number="1" location="scripts/generate_invocation_contracts.py:36">
P2: When the generator runs without the external GitHub CLI, `_load_json` raises an unhandled `FileNotFoundError` before producing bindings. Use a declared HTTP client or document and validate `gh` as a prerequisite with an actionable error.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| error_type: str | ||
| error_reason: str | ||
| dependency: Optional[str] = None | ||
| audience: Optional[ErrorAudience] = None |
There was a problem hiding this comment.
P2: The ratified audience contract requires plugin_error.audience, but this model marks it nullable. Make the field required so OpenAPI and runtime validation cannot advertise or produce incomplete plugin-error metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured_platform_plugins/etl_uvicorn/api_generator.py, line 62:
<comment>The ratified audience contract requires `plugin_error.audience`, but this model marks it nullable. Make the field required so OpenAPI and runtime validation cannot advertise or produce incomplete plugin-error metadata.</comment>
<file context>
@@ -52,6 +53,16 @@ class MessageChannels(BaseModel):
+ error_type: str
+ error_reason: str
+ dependency: Optional[str] = None
+ audience: Optional[ErrorAudience] = None
+ retryable: bool = False
+
</file context>
| audience: Optional[ErrorAudience] = None | |
| audience: ErrorAudience |
|
|
||
| def _load_json(api_path: str) -> dict[str, Any]: | ||
| completed = subprocess.run( | ||
| ["gh", "api", "-H", "Accept: application/vnd.github.raw+json", api_path], |
There was a problem hiding this comment.
P2: When the generator runs without the external GitHub CLI, _load_json raises an unhandled FileNotFoundError before producing bindings. Use a declared HTTP client or document and validate gh as a prerequisite with an actionable error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/generate_invocation_contracts.py, line 36:
<comment>When the generator runs without the external GitHub CLI, `_load_json` raises an unhandled `FileNotFoundError` before producing bindings. Use a declared HTTP client or document and validate `gh` as a prerequisite with an actionable error.</comment>
<file context>
@@ -0,0 +1,112 @@
+
+def _load_json(api_path: str) -> dict[str, Any]:
+ completed = subprocess.run(
+ ["gh", "api", "-H", "Accept: application/vnd.github.raw+json", api_path],
+ check=True,
+ capture_output=True,
</file context>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Summary
invocation_settingsplusinvocation_contextonce per/invokerequest for synchronous, asynchronous, and streaming plugins.invocation-context/v1anderrors/audience/v1contracts.invoke_with_sealed_dag_node_settings_v2capability by construction.UserErrorfailures to a complete nestedplugin_errorenvelope withaudience=user; remove the redundant top-levelblamespelling.Why
Request limits, route registration, thread-context propagation, and HTTP responses are transport concerns shared by every FastAPI-wrapped plugin. The wrapper owns those mechanics while
utic-invocation-settingsremains the authority for document validation and field-envelope resolution.Cross-service wire contracts are referenced by schema
$idand code-generated locally instead of being imported from another service package. The handwritten adapter retains only transport-specific error mapping and the equal-length batch invariant that JSON Schema cannot express.The generic transport capability never authorizes ciphertext. The controller dispatches on the document's own
formatand forwards sealed settings only when the plugin advertises the exact matching capability; v1 remains unchanged and v2 requiresinvoke_with_sealed_dag_node_settings_v2.Merge dependency
Unstructured-IO/utic-public-libs#67 is merged and
utic-invocation-settings 0.5.0is published to public PyPI. This branch now resolves that released wheel and contains no temporary source pin.utic-invocation-settings 0.5.0.Validation
uv run --locked pytest -q— 187 passeduv run --locked python scripts/generate_invocation_contracts.py --check— generated bindings match the ratified schemasuv run --locked ruff check .— cleangit diff --check— cleanPart of DTPL-648, part of DTPL-707