prepare_inputs: all three selectors, signature from the input-form descriptor - #21
Merged
Conversation
…scriptor
`prepare_inputs` took the method closure as inline `files` only, and read the target
pipe's signature from the explicit inputs template. Both halves change.
It now names the method three ways — inline `files`, a `method_ref` address resolved
by the runner, or a stored `method_id` resolved by the platform — exactly one per
call, all server-resolved with nothing expanded client-side. Empty is absent, and
none or several raises `InputPreparationError` before any request leaves the process.
The signature now comes from one `POST /v1/validate` asking for `views:
["input_form"]`, and the walk is discriminated on each descriptor node's declared
kind instead of on the shape of a value. The template marked a file position by
rendering a `{"url": …}` dict, which is a side effect of a field being *named* `url`
rather than of its concept — so an optional nested file field was never enumerated
and its local path travelled to the runner as a literal string, and a text field
merely named `url` was read from disk and uploaded. Both are gone. A `Dynamic` input
is `kind: "unknown"` and is no longer entered, which is the one deliberate behaviour
flip: such a caller uploads with `upload_file` first.
`build_inputs` and its models are deleted — the wrapper existed only to be the
signature source, and nothing calls `/v1/build/*` from this SDK now. The shared crate
envelope it also held (`MthdsFileItem`, `CrateRequestBase`, `CrateInvalidReport`)
moves to `crate_models.py`, beside the routes that still use it.
`prepare_inputs` also learns the explicit `{concept, content}` input envelope, a
pre-existing parity gap: the JS SDK has accepted it since an earlier release, so the
two would not have been identical after this fix.
Mirrors `pipelex-sdk-js` 0.17.0 (PR #42, bea4632); design of record is that repo's
wip/prepare-inputs-selectors/design.md. Verified against api-dev.pipelex.com
(pipelex-hosted 0.11.1): inline files defaulting via main_pipe, a method_ref with an
explicit pipe_ref, the manifest-only main_pipe refusal, the envelope round-trip, the
bare-pipe_ref refusal, and one real bytes upload rewritten to pipelex-storage://.
Closes L-260829-8a25d5
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HVMDoPnhuLyufT1ZoMmDTH
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Confidence Score: 4/5The wrong-typed selector handling should be fixed before merging because it can prepare inputs against a method or pipe other than the one explicitly supplied. The new normalization treats invalid selector types as absence, allowing another method selector or a default pipe to take effect silently. Files Needing Attention: pipelex_sdk/prepare_inputs.py Prompt To Fix All With AI### Issue 1
pipelex_sdk/prepare_inputs.py:109-110
**Invalid selectors become absent**
When an untyped caller passes a non-string `method_ref`, `method_id`, or `pipe_ref`, `_non_empty_string` silently converts it to `None`, allowing another method selector or the default pipe to be used and preparing inputs against a method or pipe different from the caller's explicit request.
```suggestion
if not isinstance(value, str):
msg = f"Method and pipe selectors must be strings, received {type(value).__name__}."
raise InputPreparationError(msg)
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "prepare_inputs: all three selectors, sig..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5af18a255
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Brings in v0.9.0: the `output_form` structured view on the validate report and the `mthds` pin moving 0.11.1 -> 0.13.0. One conflict, in `CHANGELOG.md`, where both sides opened a section directly under the title: this branch's `## [Unreleased]` and dev's released `## [v0.9.0]`. They describe disjoint work, so both are kept in release order. The `mthds` bump needs no adaptation here. Its breaking change makes `json_schema` required on the closed `PipeOutputContract`, and dev already fixed the one fixture in this repo that carried an output contract. The branch's own new code walks `mthds.protocol.input_form`, which is byte-identical across the two versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzVXcNRcVz8DUvAzmEuxPD
The v0.9.0 release moved the model and the pin but updated no prose, so merging it made three statements in `docs/architecture.md` false: - the `views` list was described as having one token, where `VALIDATION_VIEW_OUTPUT_FORM` now sits beside it; - the report was said to add "three typed fields", enumerated — now also `output_form` and this branch's `default_pipe_ref` (the count goes, per the workspace rule against hardcoding them); - the pipe I/O contracts claimed an output carries "no schema … the payload a run produces is the run's own result". `mthds` 0.13.0 makes `json_schema` required on `PipeOutputContract`, reversing exactly that reasoning: an output's schema is the concept's content model, declared and knowable before any run. `PipelexValidationReport`'s own docstring carried the same clause and is corrected with it. `_body_with_contracts` in the validation-contract tests was weakened by the same bump. Its output block stated no `json_schema`, so under 0.13.0 every body it built failed to parse — and its two `test_artifact_drift_fails_the_parse` cases were passing on that, not on the input drift each one names. Stating the schema restores what they test; verified by parsing a body from the helper with a clean input contract, which now succeeds. Two additions rather than corrections: the removed `build_inputs` gets a real migration target, since `mthds` 0.12.0 shipped `mthds.protocol.inputs_template` and its own `InputsTemplateFormat` — the template is relocated to the standard's package and projected client-side, not lost — and `output_form` gets its own bullet in the typed-by-import section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzVXcNRcVz8DUvAzmEuxPD
…t as absent `_non_empty_string` coerced anything that was not a string to `None`, and it served two callers that want opposite answers for that case. Reading the opaque `bundle_blueprint` — whose schema is the runtime's, not ours — a non-string really is an absent value to fall through on. Reading a CALLER's selector it is a mistake, and calling it absent made the exactly-one check unsound: `method_ref=123` beside a real `files` passed the check and prepared against a method the caller never named, `method_id=123` alone reported that no selector was given at all, and a non-string `pipe_ref` was quietly absorbed by the pipe defaulting. Split the two rules rather than raising inside the shared helper, which would have made the defensive blueprint reads throw on a shape they exist to tolerate. `_caller_selector` refuses a non-string with an `InputPreparationError` naming the argument and the type it got; `_non_empty_string` keeps the lenient contract for payload reads. `pipe_ref` is normalized beside the method selectors now, so both refusals land on the same pre-request boundary. Also corrects `prepare_inputs`'s `Raises:` section, which named `ApiResponseError` for a no-verdict `/v1/validate` failure. `validate` is 200-diagnostic and stays on the inherited `httpx.HTTPStatusError` regime, so a caller following the docstring would have missed exactly the fetch failures and 404s it listed. Advances L-260829-8a25d5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzVXcNRcVz8DUvAzmEuxPD
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
The Python twin of
pipelex-sdk-js0.17.0 (PR #42,bea4632), and this repo's step of the workspace program retiring/v1/build/*. Design of record:pipelex-sdk-js/wip/prepare-inputs-selectors/design.md; this repo's tracker iswip/prepare-inputs-selectors/plan.md.What changes
prepare_inputsnames the method three ways. Inlinefiles, amethod_refaddress (runner-resolved), or a storedmethod_id(platform-resolved) — exactly one per call, all server-resolved with nothing expanded client-side. Empty is absent; none or several raisesInputPreparationErrorbefore any request leaves the process. A method addressed by URL that declares a file input previously had no preparation path at all, even though the request beneath already accepted the address.The signature moves from the inputs template to the input-form descriptor — one
POST /v1/validatewithviews: ["input_form"]andallow_signatures=True, walked discriminated on each node's declared kind rather than on the shape of a value.Two security edges close (half of
L-260826-ddd843, P1)The template marked a file position by rendering a
{"url": …}dict — a side effect of a field being namedurl, not of its concept being an Image or Document:urlwas read from disk and uploaded.The descriptor states the resolved kind at every depth and includes optional fields.
Breaking
client.build_inputsand theBuildInputs*models are removed. The wrapper existed only to be the signature sourceprepare_inputsread; nothing calls/v1/build/*from this SDK now. (JS keptbuildInputs— it has a wrapper family retiring separately. Python only ever had this one.)MthdsFileItem,CrateRequestBase,CrateInvalidReport) moves topipelex_sdk.crate_models;build_models.pyis deleted. Models unchanged — update the import path.Dynamicinput is no longer uploaded: that input iskind: "unknown", the standard's escape hatch, and the walk does not enter it. Such a caller uploads withupload_filefirst, as the docs have always prescribed.Also in scope
prepare_inputsaccepts the explicit{concept, content}input envelope. A pre-existing parity gap rather than part of the item's letter — the JS SDK has accepted it since an earlier release and Python raisedUnsupported value at a file input … got dict— folded in with Louis's agreement so the two SDKs really are identical after this lands.Adds
PipelexValidationReport.default_pipe_ref, declared ahead of the server (L-260829-0208c7) exactly as the JS did.Verification
make agent-checkandmake agent-testgreen.Live against
api-dev.pipelex.com(pipelex-hosted0.11.1), mirroring the JS e2e:files, pipe defaulted frommain_pipemethod_ref+ explicit qualifiedpipe_refhttps://URL passed throughmethod_ref, nopipe_refmain_pipegapconceptannotation rides throughpipe_refpipelex-storage://Notes
## [Unreleased];/releasecuts the version.L-260826-ddd843closes only when both SDKs have shipped a release carrying the fix, and the JS half is still unreleased (its PR #43 is open).L-260829-60e213: a class-backed concept whose reflection cannot map a field annotation collapses tokind: "unknown", hiding a file field beneath it. Recorded indocs/input-preparation.md; deliberately not worked around here.Closes L-260829-8a25d5
Advances L-260826-ddd843
Summary by cubic
Updates
prepare_inputsto name the method three ways — inlinefiles, amethod_refaddress, or a storedmethod_id— and to read the target pipe's signature from the input-form descriptor instead of the retired/v1/build/inputstemplate (the Python half of L-260829-8a25d5 retiring/v1/build/*). The old walk classified file positions by value shape, so a text field namedurlwas read from disk and uploaded while optional nested file fields went undetected, letting local paths travel to the runner as literal strings; classifying on the descriptor's declared kind at every depth closes both security edges (L-260826-ddd843).New Features
prepare_inputsaccepts the explicit{concept, content}input envelope, closing a parity gap withpipelex-sdk-js.PipelexValidationReport.default_pipe_ref, the qualifiedpipe_refa caller gets by omitting the pipe selector; older runners omit it and callers fall back.InputPreparationErrorinstead of being treated as absent.Migration
client.build_inputsand theBuildInputs*models are removed; nothing calls/v1/build/*from this SDK now.MthdsFileItem,CrateRequestBase,CrateInvalidReport) moves topipelex_sdk.crate_models; the models are unchanged, so just update the import path.Dynamicinput is no longer uploaded; upload it withupload_filefirst.Written for commit a5ae0c4. Summary will update on new commits.