From dfb405c3cc7a993b404eccc6e07c31561f5d8342 Mon Sep 17 00:00:00 2001 From: Louis Choquel Date: Wed, 30 Sep 2026 00:21:36 +0200 Subject: [PATCH 1/2] Move the wip notes and TODOS.md to the workspace, and cite item ids The repo's wip/ notes and its tracked TODOS.md now live in the workspace's own wip repository, so the repo tracks no wip folder and the sdist stops publishing them. The citations of workspace documents become ledger item ids, and the prepare_inputs tests cite the repo's own docs/input-preparation.md for the behaviour both SDKs share. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DdKRGdm7qxgvXuspUpJLj9 --- TODOS.md | 245 ------------------ docs/architecture.md | 2 +- pipelex_sdk/crate_models.py | 2 +- tests/unit/test_prepare_inputs.py | 2 +- wip/input-form-typed-narrowing.md | 44 ---- wip/method-source-adapter/review-deferrals.md | 34 --- wip/pr-11-review-notes.md | 87 ------- wip/pr-14-review-notes.md | 63 ----- wip/pr-50-review-notes.md | 18 -- wip/prepare-inputs-selectors/plan.md | 58 ----- wip/updates.md | 176 ------------- 11 files changed, 3 insertions(+), 728 deletions(-) delete mode 100644 TODOS.md delete mode 100644 wip/input-form-typed-narrowing.md delete mode 100644 wip/method-source-adapter/review-deferrals.md delete mode 100644 wip/pr-11-review-notes.md delete mode 100644 wip/pr-14-review-notes.md delete mode 100644 wip/pr-50-review-notes.md delete mode 100644 wip/prepare-inputs-selectors/plan.md delete mode 100644 wip/updates.md diff --git a/TODOS.md b/TODOS.md deleted file mode 100644 index 6dea645..0000000 --- a/TODOS.md +++ /dev/null @@ -1,245 +0,0 @@ -# TODOS — implementing `wip/updates.md` - -This is the implementation tracker for the design in [`wip/updates.md`](wip/updates.md). The design answers *what* and *why*; this file is the *how*, broken into phases with checkboxes. Tick a box when the item is done and verified, not when it is started. Every design choice that was open has been decided (see `wip/updates.md` §7) and is treated here as settled: `input_form` stays opaque, `MethodData.python` is a typed `list[MethodFile]` with the converter in this repo, the `method_id` type guard lands now, and an unknown `FixOp.kind` raises. - -One of those settled choices has since been superseded: `input_form` is no longer opaque, and neither is `pipe_io_contracts` — both are typed by importing the standard's client models now that `mthds` publishes them. The record of that change, and why it honours rather than overrides the ownership argument the opaque ruling rested on, is [`wip/input-form-typed-narrowing.md`](wip/input-form-typed-narrowing.md). Everything else below stands as written. - -Ground rules for every phase, from `CLAUDE.md`: - -- Branch: `feature/Typed-method-id-run-option` (already carries the typed `method_id` option and the `delete_method` contract fix). The PR targets `dev`. -- Gate each phase on `make agent-check` **and** `make agent-test`; nothing is "done" before both pass. Run `make check` once at the end as well, since it adds pylint on top of the agent gate. -- Tests use `pytest-mock` only, one `TestClass` per module, no `__init__.py` under `tests/`. Mock at the httpx boundary (`_send`), as the existing modules do. -- Everything accumulates under `## [Unreleased]` in `CHANGELOG.md`. No version bump in this work: the `/release` skill cuts the version, and with the breaking items in Phase 3 (plus those already on the branch) that will be a minor bump. -- Docs move with the code, in the same commit. `docs/architecture.md` is the main one to keep truthful; its "Parity with `@pipelex/sdk`" section currently claims surface-completeness and is wrong. -- No hardcoded counts in code, docs, or commit messages. No volatile state in tracked files (this file records decisions and what landed, never whether the tree is clean or tests are green right now). -- Line numbers quoted below are as of the design date (2026-08-25) and will drift; they are anchors, not contracts. - -Suggested order is the order below. Phase 3 is the most urgent fix (a crash against the deployed platform) but is also the largest breaking change, so the plan puts the purely additive Phase 1 first to keep each commit reviewable; reorder if the crash needs to ship alone. - -## Phase 0 — preflight - -- [x] `make install` and confirm the pinned `mthds` base is the one the code was written against (`uv pip show mthds`; the floor is `>=0.8.2` and the workspace copy is 0.8.2). Nothing in this plan needs a newer `mthds`. Confirmed: 0.8.2. -- [x] Run `make agent-check` and `make agent-test` before touching anything, so a later failure is attributable to this work and not to the starting point. Both green at the starting point. -- [x] Re-read `wip/updates.md` §6 and §7 once; if anything there contradicts this file, this file is the stale one. No contradiction found. - -## Phase 1 — the `/v1/validate` surface (`wip/updates.md` §1) - -Purely additive. Every affected response model is `extra="allow"`, so nothing parses differently for a body that lacks the new keys; the work is to type what already arrives and to add the one request knob (`views`) that has no way through today. - -### 1.1 `pipelex_sdk/validation_models.py` — the fix vocabulary and the new fields - -- [x] Add `FixSafety(StrEnum)` with `SAFE = "safe"`, `UNSAFE = "unsafe"`, and an `is_safe` property (house rule: never compare enum values inline; a `match` inside the property). -- [x] Add `FixOpKind(StrEnum)` with the kinds the runtime emits: `SET_KEY = "set_key"`, `ENSURE_TABLE = "ensure_table"`, `DELETE_KEY = "delete_key"`, `DELETE_TABLE = "delete_table"`, `RENAME_TABLE_KEY = "rename_table_key"`, `MOVE_KEY = "move_key"`, `REMAP_VALUE = "remap_value"`. Source of truth: `pipelex/pipelex/suggested_fix.py` and the OpenAPI artifact `pipelex-api/docs/openapi/pipelex-api.openapi.yaml`. -- [x] Add `TomlScalar: TypeAlias = str | int | float | bool` and `TomlValue: TypeAlias = TomlScalar | dict[str, TomlScalar]`. Deeper nesting is not modelled because the server does not emit it; say so in a comment. -- [x] Add a private `_FixOpBase(BaseModel)` with `model_config = ConfigDict(extra="allow")` and `table_path: list[str]` (empty list means the document root), then one subclass per kind, each with `kind: Literal[FixOpKind.X]` and exactly its own members: `SetKeyOp(key: str, value: TomlValue)`, `EnsureTableOp()`, `DeleteKeyOp(key: str)`, `DeleteTableOp()`, `RenameTableKeyOp(key: str, new_key: str)`, `MoveKeyOp(key: str, new_table_path: list[str], new_key: str)`, `RemapValueOp(key: str, mapping: dict[str, str])`. On `EnsureTableOp` and `DeleteTableOp` declare `table_path: list[str] = Field(min_length=1)`, mirroring the artifact's `minItems: 1`. -- [x] These are **reader** models: no `frozen`, no `extra="forbid"`, none of the runtime's wildcard-refusing validators. Put the two runtime invariants a type cannot carry in the docstrings (`*` is the wildcard segment and is refused as a `key` on every kind but `remap_value`; `ensure_table` / `delete_table` need a non-empty `table_path`). -- [x] Add `FixOp: TypeAlias = Annotated[SetKeyOp | EnsureTableOp | DeleteKeyOp | DeleteTableOp | RenameTableKeyOp | MoveKeyOp | RemapValueOp, Field(discriminator="kind")]`. Narrowing is `match op: case SetKeyOp(): …`, exhaustive, no `case _`. -- [x] **Verify** that pydantic accepts the raw wire string (`"set_key"`) against `Literal[FixOpKind.SET_KEY]` both in `validate_python` and `validate_json`, and that the discriminator resolves on it. If it does not on the pinned pydantic, use `Literal["set_key"]` on the models and keep `FixOpKind` as the documented vocabulary (with a test that the two sets agree). -- [x] Add `SuggestedFix(BaseModel, extra="allow")`: `fix_code: str`, `description: str`, `safety: FixSafety`, `source: str | None = None`, `ops: list[FixOp]` (typed default factory via `empty_list_factory_of` only if a default is warranted — the runtime always sends `ops`, so leaving it required is fine). -- [x] Add `LiftablePipeEntry(BaseModel, extra="allow")`: `pipe_ref: str`, `within_pipe_ref: str`, `skipped_when_absent: list[str] = Field(default_factory=list)`, `absence_source: str`. Mirrors `pipelex/pipelex/pipeline/liftable_pipes.py`. -- [x] Add the view token constant next to the field it gates: `VALIDATION_VIEW_INPUT_FORM: Final[str] = "input_form"`. A constant, not a closed enum — the request boundary is deliberately open so a stale token never fails a call. -- [x] On `ValidationErrorItem` add `missing_pipe_code: str | None = None` (symmetrical with `missing_concept_code`) and `suggested_fix: SuggestedFix | None = None`. Leave `error_type: str | None` as an open string; do not enum it. -- [x] On `PipelexValidationReport` add `warnings: list[ValidationErrorItem] = Field(default_factory=empty_list_factory_of(ValidationErrorItem))`, `liftable_pipes: list[LiftablePipeEntry] = Field(default_factory=empty_list_factory_of(LiftablePipeEntry))`, and `input_form: dict[str, Any] | None = None`, each with a docstring: `warnings` never flips `is_valid`; the two lists default empty so a pre-0.52 runner's body still parses; `input_form` is present only when the request named the `input_form` view (a 0.17.0 runner emitted it unconditionally, which `None`-by-default also reads correctly) and is opaque on purpose, keyed like `pipe_io_contracts`. -- [x] `PipelexInvalidReport` gains nothing; add one sentence to its docstring saying why (the invalid arm never carries `warnings` or `input_form` — they derive from a crate that was never assembled). -- [x] Update the module docstring's list of neutrally-named supporting types to include the new ones, and keep the brand rule stated there (the `Pipelex` prefix stays on the two envelopes only). - -### 1.2 `pipelex_sdk/client.py` — `views` on `validate` and `validate_files` - -- [x] `validate(...)`: append `views: list[str] | None = None` after `render`. When `views is not None`, set `extra["views"] = views` **verbatim** — no injection, no de-duplication, an explicit `[]` is sent as `[]`. When `None`, the key is absent from the body. It rides `_post_validate`'s `extra` exactly like `render` and `mthds_sources`; no `mthds-python` change. -- [x] `validate_files(...)`: append `views: list[str] | None = None` after `render` and thread it through to `validate`. -- [x] Docstrings: the sentence "differs from the inherited protocol `validate` in two Pipelex-API ways" becomes three (render injection, `mthds_sources`, `views`); document the `views` semantics (opt-in structured views; `input_form` is the only token today, named by `VALIDATION_VIEW_INPUT_FORM`; unknown tokens are lenient-ignored server-side, never a `422`; the default response stays byte-identical for consumers that discard views). -- [x] The `Returns:` section of `validate` should mention that a valid report now carries `warnings`, `liftable_pipes`, and (when asked) `input_form`. - -### 1.3 Tests - -- [x] `tests/unit/test_client_validate.py`: `views` sent verbatim when given; the `views` key absent from the body when the parameter is omitted; an explicit `[]` sent as `[]`; `validate_files` threads `views` through; `render` injection unchanged when `views` is also passed. -- [x] `tests/unit/test_validation_contract.py`: a valid body carrying `warnings`, `liftable_pipes`, and `input_form` parses into typed fields, with `input_form` keyed like `pipe_io_contracts`; the pre-0.52 `VALID_BODY` still parses with both lists empty and `input_form` `None`; the JS null-bearing warning fixture (`pipelex-sdk-js/tests/client.test.ts`, "carries advisory warnings on the VALID arm, with the valid arm's explicit nulls") parses with every explicit `null` reading as `None` — this is the regression guard against a future "tighten to required" edit, and it answers the inbox item `../wip/inbox/2026-08-25-workspace-validation-error-item-spec-gaps.md` for the Python mirror. -- [x] `tests/unit/test_validation_contract.py`: an invalid body carrying `missing_pipe_code` and a `suggested_fix` with at least two ops of different kinds parses, and `match`-narrowing reaches each op's own members; an `ensure_table` op with an empty `table_path` is rejected; an unknown `kind` raises `pydantic.ValidationError`; `FixSafety` and `FixOpKind` value sets are asserted as the locked vocabularies (same style as `test_category_vocabulary_is_the_locked_set`). -- [x] Keep the canonical bodies where the module already keeps them (module-level constants next to `VALID_BODY`); move to a `tests/unit/test_data.py` only if the module becomes unreadable. - -### 1.4 Docs and changelog - -- [x] `docs/architecture.md` → "`validate` override": add a `views` bullet beside the render bullet; list the typed valid-arm additions (`warnings`, `liftable_pipes`, `input_form`) and the `ValidationErrorItem` additions with the `SuggestedFix` / `FixOp` / `FixSafety` vocabulary; one sentence that `PipeInputContract.optional` became `presence` and that the `fixed` multiplicity carries `item_count` inside the opaque `pipe_io_contracts`, so nobody discovers the new spellings by surprise. -- [x] `docs/architecture.md` → "Brand boundary": the list of neutrally-named supporting types gains the new ones. -- [x] `README.md` quickstart: one line showing `views=[VALIDATION_VIEW_INPUT_FORM]` (or a comment that the input form is opt-in), so the knob is discoverable. -- [x] `CHANGELOG.md` `[Unreleased]` → **Added**: `views` on `validate` / `validate_files`; the typed valid-arm fields; `missing_pipe_code` / `suggested_fix` and the fix vocabulary; a note that a body from an older runner still parses (the lists default empty, `input_form` defaults `None`). - -### 1.5 Gate and commit - -- [x] `make agent-check` and `make agent-test` pass. -- [x] Commit (suggested message: "Type the pipelex-api 0.17/0.18 validate contract and add the views opt-in"). - -### Checkpoint 1 - -- [x] Update this file: tick what landed, record the SHA of the Phase 1 commit, note whether pydantic accepted the enum `Literal` tags or the string fallback was needed, and any deviation from §1 of the design with its reason. - -**Landed in `434b2e3`.** Notes: - -- **The enum `Literal` tags work as written; the string fallback was not needed.** Verified against the pinned pydantic (2.13.4) before writing the models: a raw wire `"set_key"` validates against `Literal[FixOpKind.SET_KEY]` in both `validate_python` and `validate_json`, the discriminator resolves on it, an unknown `kind` raises, and `Field(min_length=1)` on `EnsureTableOp.table_path` rejects an empty path. -- **`RemapValueOp.mapping` is left unconstrained**, where the runtime and the OpenAPI artifact both declare `minProperties: 1`. These are reader models: an empty mapping is an advisory no-op, not a parse hazard, and refusing it would fail a whole verdict over a harmless op. The `minItems: 1` on `ensure_table` / `delete_table` was kept because there the empty case is genuinely meaningless (the document root always exists, and cannot be deleted). -- **One Phase-2 item landed early**, because `validation_models.py` was rewritten wholesale here: the `conformance/conformance/validation_contract.py` citation on `ValidationErrorCategory` is already replaced with the rule it was citing. Phase 2 covers the rest. -- No other deviation from §1 of the design. - -## Phase 2 — prose corrections (`wip/updates.md` §2) - -No behaviour change. Two fixes `@pipelex/sdk` 0.14.0 shipped under "Fixed" that apply here for the same reason (`pipelex-sdk` is a public PyPI package). - -- [x] **`TokensUsageRecord` attribution.** `pipelex_sdk/runs.py` (module docstring near line 23 and the class docstring near line 128), `docs/run-usage.md` (line 5), `docs/architecture.md` (the `TokensUsageRecord` bullet near line 103): the record is a Pipelex runtime extension the MTHDS Protocol does not model, and the hosted API pins the wire contract. Reword as `pipelex-sdk-js/src/runs.ts` and its `docs/architecture.md` did. `docs/run-usage.md` line 5 already says the right thing in its second sentence; make the first sentence agree with it. -- [x] **Citations a reader cannot open.** Replace each bare workspace-private path with the rule it was citing: `pipelex_sdk/client.py` near line 138 (`docs/specs/pipelex-platform-api.md` → "the layered extension policy: a hosted client types its own platform's arguments and guards them per layer"); `pipelex_sdk/validation_models.py` near line 44 (`conformance/conformance/validation_contract.py` → "the locked category vocabulary shared with the conformance corpus"); `tests/unit/test_validation_contract.py` module docstring and the docstring of `test_category_vocabulary_is_the_locked_set`; `tests/unit/test_runs.py` near line 12; `tests/unit/test_client_method_id.py` module docstring; `docs/architecture.md` near line 84. Keep the JS mirror references (`pipelex-sdk-js/...`) where they explain a port — those are a sibling public repo, not a private path. -- [x] `CHANGELOG.md` `[Unreleased]` → **Fixed**: two entries mirroring 0.14.0's wording. -- [x] `make agent-check` and `make agent-test` pass. -- [x] Commit (suggested message: "Correct the TokensUsageRecord attribution and drop unopenable citations"). - -## Phase 3 — product paging and nullability (`wip/updates.md` §3) - -Breaking, and the most urgent fix in this plan: `list_methods` and `list_runs` crash against the deployed platform because both routes now answer a `{items, next_cursor}` envelope, and `PipelineRun` requires fields the platform serves as nullable. Wire fields stay snake_case (`next_cursor`), where the JS mirror renamed to `nextCursor` for its own consumers. - -### 3.1 `pipelex_sdk/product_models.py` — models - -- [x] Add `MethodFile(BaseModel, extra="allow")` with `name: str`, `content: str`, defined **before** `MethodData`. Docstring: the at-rest catalog form of one source file (`[{name, content}]`), distinct from `MthdsFile` (`client.py`, validate input) and `MthdsFileItem` (`build_models.py`, build closure) — three shapes for three surfaces, name the difference so nobody merges them. -- [x] Add `parse_method_files(source: str | None) -> list[MethodFile]`: blank source (`None`, `""`, whitespace) and `"[]"` both yield `[]`; a JSON array of `{name, content}` yields those files with blank-content entries dropped; anything else (non-array JSON, a malformed entry, unparseable text) raises `ValueError` with a message naming the expected shape. Implementation: `json.loads` then `TypeAdapter(list[MethodFile])` (built once at module level, TypeAdapter construction is expensive), wrapping `json.JSONDecodeError` / `pydantic.ValidationError` into the `ValueError`. -- [x] Add `serialize_method_files(files: list[MethodFile]) -> str`: drop blank-content entries; an empty result serializes to `""` (the platform's "no source / clear" sentinel), never `"[]"`; otherwise `json.dumps` of `[{name, content}]` only (no extras), stable key order. -- [x] `MethodData`: add `org_id: str`, `created_by_user_id: str`, `description: str | None = None`, `deletion_state: MethodDeletionState | None = None`, `python: list[MethodFile] = Field(default_factory=empty_list_factory_of(MethodFile))`, plus a `@field_validator("python", mode="before")` that applies `parse_method_files` when the incoming value is a `str` or `None` and passes a list through unchanged (so programmatic construction in tests still works). A `ValueError` from the parser surfaces as `pydantic.ValidationError` from `model_validate`, the same way any malformed response body fails here. -- [x] `MethodWriteInput`: add `python: list[MethodFile] | None = None` with a `@field_serializer("python")` returning `serialize_method_files(value)`. Docstring the three-way contract: `None` → key absent (the write body dumps with `exclude_none=True`) → the stored Python is preserved on `PUT`; `[]` → sent as `""` → clears it; a non-empty list → replaces it. -- [x] Add `MethodSummary(BaseModel, extra="allow")`: `method_id: str`, `name: str`, `description: str | None = None`, `created_at: str`, `deletion_state: MethodDeletionState | None = None`. Docstring: deliberately not a `MethodData` — no `mthds`, no `python`, no `updated_at` — because none is in the index projection and putting `mthds` back is what restored the truncation bug; a method mid-deletion stays in the list so a UI can render "Deleting…" while `get_method` refuses it with a `409`. -- [x] Add `MethodPage(BaseModel, extra="allow")`: `items: list[MethodSummary]`, `next_cursor: str | None = None`. Docstring: opaque cursor, pass it straight back; `None` means last page; no total by design. -- [x] Add `RunErrorReport(BaseModel, extra="allow")`: `message: str | None = None`, `error_type: str | None = None` — the two fields a consumer may rely on out of the runner's verbose report. -- [x] `PipelineRun`: `method_id: str | None = None` (an ad-hoc run from an inline bundle belongs to no stored method), `pipe_code: str | None = None` (resolved from the bundle's `main_pipe`); add `org_id: str | None = None`, `created_by_user_id: str | None = None`, `error: RunErrorReport | None = None`. Leave `pipe_statuses` as it is. -- [x] Add `RunDetail(PipelineRun)`: `mthds_contents: list[str] | None = None`, `inputs: dict[str, Any] | None = None`. Docstring: `mthds_contents` is what the run actually executed and the only record of it; both fields are absent from the list and the polled status read on purpose (size × page size, size × poll rate). -- [x] Add `RunPage(BaseModel, extra="allow")`: `items: list[PipelineRun]`, `next_cursor: str | None = None`. -- [x] Update the section comments in the module (`# ── Methods catalog`, `# ── Run records`) so the new models sit under the right banner. - -### 3.2 `pipelex_sdk/errors.py` — the runaway-paging error - -- [x] Add one error for `iterate_methods` refusing to keep paging past the ceiling (a name like `PagingNotTerminatingError`), extending whatever base the module's other product errors extend — check the existing hierarchy there first. Message mirrors the JS one: the iterator did not terminate after the ceiling; this is a server-side fault, not a coverage limit. - -### 3.3 `pipelex_sdk/client.py` — list, iterate, detail - -- [x] Add a module helper `_product_query(params: dict[str, str | int | None]) -> str` that keeps entries on **presence** (`is not None`, never truthiness — an explicit empty `q` or cursor is bad input the API should reject, not something to drop silently) and encodes with `urllib.parse.urlencode`, returning `""` or `?…`. The existing `test_list_runs_encodes_query_value` assertion (`method_id=m%2F1`) must stay green, so keep `/` percent-encoded. -- [x] Add `_MAX_LIST_PAGES: int = 10_000` beside the other module constants (the JS `MAX_PAGES`), with the comment that it is a runaway backstop set far beyond any real catalog, not a coverage cap. -- [x] `list_methods(self, *, q: str | None = None, limit: int | None = None, cursor: str | None = None) -> MethodPage` — `GET /v1/methods` with the query built by the helper; parse `MethodPage`. Docstring: `q` is a server-side case-insensitive substring match over name and description across the whole catalog; `limit` defaults to and is capped by the API; ordering is by creation, newest first. -- [x] `iterate_methods(self, *, q: str | None = None, limit: int | None = None) -> AsyncIterator[MethodSummary]` as an `async def` generator: request a page; **before yielding**, stop if `cursor is not None and page.next_cursor == cursor` (the server did not advance; yielding first would double-count); yield every item; stop when `page.next_cursor is None`; otherwise count the page and **raise** the new error once the count reaches `_MAX_LIST_PAGES`; continue **through empty pages** with a live cursor, because `q` is a post-read filter over a bounded index slice per request and `{items: [], next_cursor: "…"}` means "keep going". Docstring says why there is no `list_all_methods()`: an all-at-once helper needs a cap, and a cap is the silent truncation paging removed. -- [x] `list_runs(self, method_id: str, *, created_from: str | None = None, created_to: str | None = None, limit: int | None = None, cursor: str | None = None) -> RunPage` — `GET /v1/runs?method_id=…` plus the presence-kept query; parse `RunPage`. Docstring: `created_from` / `created_to` are instants (ISO-8601 with a UTC offset), inclusive, key conditions rather than filters; a bare date or naive timestamp is a platform `400` surfaced as `ApiResponseError`. Also document the gate the JS mirror does not spell out: every `/v1/runs*` product route sits behind the platform's `require_surface_access()`, which for API-key auth demands the `ff_api_keys` feature flag and fails closed with a `403` — a `403` here means "flag", not "wrong key". -- [x] `iterate_runs(self, method_id: str, *, created_from: str | None = None, created_to: str | None = None, limit: int | None = None) -> AsyncIterator[PipelineRun]` — same loop, except an **empty page ends it** (date bounds are index key conditions, so a run page is never empty-with-a-cursor; the difference is the server, not the client — say so in the docstring). ~~No page ceiling needed: the empty-page stop already catches a server minting fresh cursors while returning nothing.~~ **Deviation, taken on PR review:** the ceiling applies here too. The plan's reason was too narrow — the empty-page stop only catches a server returning *nothing*, so a cursor cycling across two or more values (`c1 → c2 → c1`) over non-empty pages trips neither it nor the adjacent-cursor check and would loop forever re-yielding the same runs. Greptile and Codex flagged it independently. The fix reuses `_MAX_LIST_PAGES` and `PagingNotTerminatingError` rather than tracking every cursor seen, which would cost unbounded memory for the same protection. -- [x] `get_run_detail(self, run_id: str) -> RunDetail` — `GET /v1/runs/{id}` via `_request_product` with the id path-encoded like the other id routes (`f"{_RUNS}/{quote(run_id, safe='')}"`). Distinct from `get_run_status` (`/status`) and `get_run_result` (`/results`). -- [x] Update the `PipelexAPIClient` class docstring's product-surface bullet and the import block (`MethodPage`, `MethodSummary`, `RunDetail`, `RunPage`, `AsyncIterator` from `collections.abc` under `TYPE_CHECKING` if only used in annotations — it is used at runtime as a return annotation with `from __future__ import annotations`, so `TYPE_CHECKING` is fine). - -### 3.4 Tests - -- [x] `tests/unit/test_client_product.py`: replace the bare-array fixtures at `test_list_methods` and `test_list_runs_encodes_query_value` with envelopes and assert `MethodPage` / `RunPage` come back with `next_cursor`; add query-encoding cases for `q` / `limit` / `cursor` and for `created_from` / `created_to`, including that an explicit empty string is forwarded rather than dropped and that an absent parameter leaves no key in the query; a run row with `null` `pipe_code` and `method_id` parses; `get_run_detail` hits `/v1/runs/{id}` with encoding and returns `mthds_contents` and `inputs`; `MethodData` parses the new fields with `python` read from the wire string into `MethodFile` entries and `""` reading as `[]`; `create_method` / `update_method` send `python` three ways (`None` absent, `[]` as `""`, a list as the JSON text). -- [x] New `tests/unit/test_method_files.py` (one `TestClass`): `parse_method_files` on blank / `"[]"` / a valid array / an array with a blank-content entry / a non-array / a malformed entry / unparseable text; `serialize_method_files` on empty / blank-only / mixed; a round-trip is stable. -- [x] New `tests/unit/test_client_paging.py` (one `TestClass`): `iterate_methods` continues through an empty page with a live cursor and stops on `None`; both iterators stop on an unchanged cursor without re-yielding the page; `iterate_runs` stops on an empty page; `iterate_methods` raises the new error past the ceiling (patch `_MAX_LIST_PAGES` down via `mocker.patch` rather than looping ten thousand times); the cursor sent on page N+1 is the `next_cursor` received on page N. Use `mocker.AsyncMock(side_effect=[...])` on `_send` to script the page sequence. -- [x] The `_response` / `_Sent` / `_mock_send` helpers live as private members of `tests/unit/test_client_product.py`. Rather than importing private test helpers across modules, promote a response builder and a `_send` spy to fixtures in a new `tests/unit/conftest.py` for the new modules to use (house rule: fixtures go in `conftest.py`). Migrating `test_client_product.py` onto those fixtures is optional and not part of this change. - -### 3.5 Docs and changelog - -- [x] `docs/architecture.md` → "Pipelex product surface": rewrite the **Methods catalog** bullet for `MethodPage` / `MethodSummary` / `iterate_methods` and the `python` three-way write contract with `MethodFile`; rewrite the **Run records** bullet for `RunPage` / `iterate_runs` / `get_run_detail`, the nullable `PipelineRun` fields and `error`, the instant-only date bounds, and the `ff_api_keys` `403`. State the two iterator stop rules and why they differ. -- [x] `docs/architecture.md` → "Parity with `@pipelex/sdk`": it must stop claiming "surface-complete, with no silent gaps". Rewrite it to list the conscious exclusions honestly: `lint`, `format`, `resolve`, `codegen`, `build_output` / `build_runner` / `concept` / `pipe_spec`, `run_codegen_check`, `get_method_closure` — unchanged by the cited releases and deferred. While there, fix the stale "Out of scope for v0.1" bullet that still lists `/v1/build/*` helpers as deferred even though `build_inputs` shipped in 0.5.0. -- [x] `README.md`: if the quickstart gains a listing example, use `iterate_methods` rather than a page loop, so the idiom people copy is the one that cannot truncate. -- [x] `CHANGELOG.md` `[Unreleased]`: **Changed (breaking)** — `list_methods` returns `MethodPage` (items are `MethodSummary`, not `MethodData`), `list_runs` returns `RunPage`, `PipelineRun.method_id` / `pipe_code` are nullable, `MethodData.python` is `list[MethodFile]`; **Added** — `iterate_methods`, `iterate_runs`, `get_run_detail`, `MethodSummary` / `MethodPage` / `RunPage` / `RunDetail` / `RunErrorReport`, `MethodFile` with `parse_method_files` / `serialize_method_files`, the new `MethodData` fields, `MethodWriteInput.python`, the paging error; **Fixed** — name the crash plainly (iterating the envelope dict yielded its keys, so the first call was `MethodData.model_validate("items")`), and that the unit tests mocked the pre-paging shape. - -### 3.6 Gate and commit - -- [x] `make agent-check` and `make agent-test` pass. -- [x] Commit (suggested message: "Follow the platform's paged method and run lists and stop requiring nullable run fields"). - -### Checkpoint 2 - -- [x] Update this file: tick what landed, record the Phase 2 and Phase 3 commit SHAs, and note any place the Python shapes deliberately diverge from the JS mirror beyond snake_case (there should be none besides the `python` converter living here). - -**Phase 2 landed in `2a8c589`, Phase 3 in `5e01c1b`.** Notes: - -- **Divergences from the JS mirror beyond snake_case:** the `python` converter lives here rather than in `mthds-python` (the decision of `wip/updates.md` §7.2), and `parse_method_files` raises `ValueError` where the JS pair raises `PipelineRequestError` — because the Python parser is reached through a pydantic `field_validator`, where a `ValueError` is the idiomatic signal and surfaces to the caller as a `pydantic.ValidationError` like any other malformed response body. Nothing else diverges. -- **`MethodPage.items` / `RunPage.items` are required**, not defaulted empty. A page body with no `items` is malformed, and failing loudly is the whole point of this phase — the previous shape failed silently in the tests and loudly in production. -- **The shared test fixtures landed in a new `tests/unit/conftest.py`** (`api_client`, `wire_response`, `patch_send`), used by the two new modules. Migrating `test_client_product.py` onto them was left out as the plan allowed; it keeps its own equivalent private helpers. - -## Phase 4 — `method_id` boundary type guard (`wip/updates.md` §4) - -The 2026-08-25 decision in `pipelex-sdk-js/wip/boundary-option-type-validation.md`: a published client validates request-option types at its boundary and raises `PipelineRequestError` rather than dropping or forwarding a wrong-typed value. Its evidence names this repo's bare `if method_id:` in `_merge_hosted_run_extensions`, which drops falsy non-strings and forwards truthy ones to a server `422`. - -- [x] `_merge_hosted_run_extensions` (`client.py` near line 975): replace `if method_id:` with an explicit presence check (`if method_id is not None`) followed by `if not isinstance(method_id, str): raise PipelineRequestError(msg)` naming the received type, then the existing empty-string-is-absent normalization. `None` and `""` still contribute nothing. -- [x] Update the docstring's `Raises:` and the "An absent or empty `method_id`" paragraph. -- [x] `tests/unit/test_client_method_id.py`: one parametrized test over wrong-typed values (`0`, `123`, `[]`, `["mt_1"]`, `{}`) asserting `PipelineRequestError` on `execute` and `start` before any request is sent; keep `test_empty_method_id_is_absent` green. -- [x] `docs/architecture.md` → "Hosted run extensions (`method_id`)": add a bullet that a non-string raises at the boundary, and why (one partition of wrong values across both SDKs). -- [x] `CHANGELOG.md` `[Unreleased]` → **Changed**: the guard, with the JS decision as the reason. -- [x] `make agent-check` and `make agent-test` pass. -- [x] Commit (suggested message: "Reject a non-string method_id at the client boundary"). - -## Phase 5 — wrap-up - -- [x] `make check` (adds pylint to the agent gate) and `make agent-test` pass on the final tree. -- [x] Re-read `docs/architecture.md` end to end for any remaining claim the code no longer supports (parity, `Out of scope`, the validate section, the product section). Three further corrections beyond the ones §3.5 named: the intro line claiming "the full `0.1.0` surface", the parity section's "**Methods** — full coverage" (which contradicted the gap list directly above it), and the "**Models** — full field-for-field match" claim; the Models paragraph now also names the two deliberate divergences (snake_case `next_cursor`, the converter's home). -- [x] Re-read `CHANGELOG.md` `[Unreleased]`: every breaking item is labelled "breaking", no counts, no WIP-doc mentions, the version line untouched (still `0.5.0`). -- [x] `wip/updates.md` stays where it is, with its §7 decisions; this file stays too. Neither is deleted or emptied as part of finishing the work. -- [x] Open the PR against `dev`, then follow `/review-pr-agents` for the Greptile / Codex loop (compare SHAs, not notifications; reply and resolve each thread in one pass). **PR [#14](https://github.com/Pipelex/pipelex-sdk-python/pull/14).** - -### Checkpoint 3 - -- [x] Update this file with the final commit SHAs, the PR number, and anything deferred out of the plan with the reason. - -**PR [#14](https://github.com/Pipelex/pipelex-sdk-python/pull/14), against `dev`.** The five implementation commits, in order: - -| SHA | What | -|---|---| -| `434b2e3` | Phase 1 — the validate surface: the `views` opt-in and the typed 0.17/0.18 contract | -| `2a8c589` | Phase 2 — the `TokensUsageRecord` attribution and the unopenable citations | -| `5e01c1b` | Phase 3 — paged method and run lists, nullable run fields, the `python` converter | -| `cb70fbe` | Phase 4 — the non-string `method_id` boundary guard | -| `a857d02` | Phase 5 — the remaining untrue parity claims in `docs/architecture.md` | - -Deferred out of the plan, with the reason: - -- **`RemapValueOp.mapping` is not constrained to be non-empty**, where the runtime and the OpenAPI artifact both say `minProperties: 1`. Reader models should not fail a whole verdict over an op that is merely a no-op. Recorded at Checkpoint 1. -- **Migrating `test_client_product.py` onto the new `tests/unit/conftest.py` fixtures** was explicitly optional in §3.4 and was not done; the module keeps its own equivalent private helpers. -- **`pipelex_sdk/runs.py`'s "Wire contract mirrors `pipelex-platform`" line** was left alone. It names a service, not a file path, so it is not one of the unopenable citations Phase 2 was about, and widening that phase's scope on my own judgement was not warranted. - -### Corrections found on PR review - -Cubic's pass on `fa8d62a` reported findings against files this plan touched. Three were real and are fixed; the rest were judged not worth acting on, and the reasons are recorded here rather than only in the resolved GitHub threads. - -Fixed: - -- **The `TokensUsageRecord` attribution in `docs/architecture.md` was missed.** Phase 2's first checkbox names that site explicitly, and `CHANGELOG.md` asserts it was corrected, but `2a8c589` only touched the `method_id` citation in that file. `pipelex_sdk/runs.py` and `docs/run-usage.md` were correct. The bullet now carries the same wording as the other two. -- **`CHANGELOG.md` still cited `docs/specs/pipelex-platform-api.md`.** The changelog was outside Phase 2's enumerated citation sites, so the sweep did not reach it — leaving the `[Unreleased]` section claiming "no more citations a reader cannot open" a few entries below a citation a reader cannot open. The sentence now states the rule and points at the sibling public JS SDK instead. -- **The `validate` override intro in `docs/architecture.md` still said "two Pipelex-API extensions"** after Phase 1 added the third. The client docstring was updated at the time; the doc's intro sentence was not. - -Not acted on, with the reason: - -- **Hoisting the `method_id` type guard above `start_and_wait`'s lifecycle handshake.** The claim that a handshake failure can mask the guard is wrong: `_supports_run_lifecycle` swallows its errors and every downstream path still reaches `_merge_hosted_run_extensions`, so no wrong-typed value escapes. The real cost is one probe request, memoized for the client's lifetime. Hoisting would mean either calling the merge helper twice or duplicating the check outside the single documented guard site. -- **A claimed 194-character line in `tests/unit/test_client_validate.py`.** Measured at 127, under the configured 150, and no line in that file is longer; `ruff check` passes. The measurement was simply wrong. -- **Rewriting Checkpoint 2's parenthetical to match the two divergences the note beneath it records.** The parenthetical is the expectation the plan set out with, and the note is the finding; editing the prediction to match the outcome removes the only evidence that the plan's expectation was slightly off. -- **The `../wip/inbox/…` reference in §1.3.** Phase 2's rule is about the *shipped* surface — docstrings, comments and doc pages that travel to PyPI, where a workspace path resolves to nothing and reads as rot. This tracker is not that: it is a working document for the reviewers of this PR, it is written from workspace context throughout (it cites `wip/updates.md` and the JS repo's own `wip/` in the same way), and `../wip/inbox/` is the notation the workspace guide itself prescribes for a sub-repo checkout. The path resolves where the document is read, and the sentence already states its rationale inline before citing the item, so a reader who cannot open it loses only the provenance. -- **Replacing the two `# type: ignore[arg-type]` comments in `tests/unit/test_client_method_id.py` with `cast()`.** A cast would assert to the reader that the value *is* a `str | None`, which is the exact falsehood that test exists to disprove at runtime; the ignore states the truth that this is a deliberate type error, which is the last resort the coding standard permits. - -### Final pre-landing review - -Once the three PR bots reported clean on `301d96e`, a fresh reviewer with no context from this session ran the `gstack` review procedure against this plan, with an adversarial pass folded in. It found no correctness defect in the shipped code, and it verified the wire contracts against `pipelex-server` itself rather than against the docstrings that assert them — including that the two iterators' asymmetric stop rules match the two DynamoDB adapters (the method adapter can mint a cursor for an empty page because `q` filters after the read, the run adapter over-fetches by one and cannot), and that the pydantic three-way write contract for `python` holds for absent / `null` / `""` / `"[]"` alike. - -Fixed: - -- **`README.md` advertised an import that raises `ImportError`.** "Public import paths" sent readers to `from mthds.runners.api.models import PipelexValidationResult`; that name is not in the installed `mthds` at all. The Pipelex narrowing of the verdict union is owned here, which `docs/architecture.md` states in its "Brand boundary" section — so the repo contradicted itself. The section gains a `pipelex_sdk.validation_models` bullet and now cites `mthds.protocol.models.ValidationResult` as the neutral union it narrows. Pre-existing, but this branch edited the quickstart three lines above it. -- **The two page-ceiling tests could not fail.** Their only assertion was `exc_info.value.page_limit`, which just echoes back the constant the test itself patched, so five scripted responses against a limit of two passed whether the raise fired on page 1 or page 5. Each now also asserts `send.call_count`, which was mutation-checked: loosening `pages_seen >= _MAX_LIST_PAGES` to `>` fails both tests where it previously passed. -- **Two hardcoded counts that had already gone stale inside this branch.** `pipelex_sdk/client.py`'s `validate` docstring and `docs/architecture.md` both said "three Pipelex-API ways/extensions" — the same rot the entry above records having corrected once already, which is exactly why the workspace guide forbids writing counts down. Both now say neither two nor three. -- **`execute` and `start` under-documented an exception this branch added.** Their `Raises:` entries named only the `extra`-smuggling trigger; Phase 4's guard raises `PipelineRequestError` for a non-string `method_id` too. The helper's own docstring had been updated at the time, the two public methods' had not. - -Deferred to `wip/pr-14-review-notes.md`, each verified and none blocking: the sdist ships this tracker and the other internal planning documents (a packaging decision that belongs with a release, not inside a feature branch); the client class docstring still dates two surfaces by build-plan phase number; and `start_and_wait`'s `Raises:` omits the `PipelineRequestError` it propagates on both paths. - -One finding reached outside this repo and was filed rather than fixed: `PipelineRun.pipe_statuses` is a field no server has ever filled — it is absent from the platform's `RunPublic` response model and appears nowhere in `pipelex-server` — yet it is declared in this SDK, in `@pipelex/sdk`, and in `pipelex-app`, where run-history progress dots are gated on it and have therefore never rendered. Retiring it here alone would break the parity invariant this package is built on, so the decision belongs to whoever owns the run wire contract: `../wip/inbox/2026-08-25-workspace-pipe-statuses-dead-field-in-three-clients.md`. - -Considered and declined: guarding `iterate_methods` against a `next_cursor` of `""`, which would let a page double-yield. The adversarial pass demonstrated the mechanism and then confirmed the case is unreachable, since the platform's cursor is a base64 `LastEvaluatedKey`. That is the impossible-scenario defensiveness this plan set out to avoid. - -Cubic's pass on `cb391c9` reported four more. Two were real: - -- **`docs/architecture.md` documented an API that does not exist.** The hosted-run-extensions section contrasted `method_id` with "`build_inputs(method_id=…)` and `prepare_inputs`, which are client-side sugar that resolves an id to inline files". Neither helper accepts a `method_id`: `build_inputs` takes a `BuildInputsRequest` of `files` / `pipe_ref` / `format` / `explicit`, `prepare_inputs` takes `files` / `pipe_ref` / `inputs`, and catalog-id resolution for input preparation is recorded as deferred in the 0.5.0 changelog. A reader following that sentence gets a `TypeError`. The contrast it wanted to draw is real, so the bullet now draws it truthfully. This is exactly the class of untrue claim Phase 5 set out to remove from this file, in a section Phase 4 added. -- **This tracker recorded the wrong `mthds` floor.** Phase 0 wrote `>=0.8.1`; `pyproject.toml` has said `>=0.8.2` since `bc17c07`, which is an ancestor of this branch's base — so the floor was already 0.8.2 when the preflight ran, and the number was wrong the day it was written. - -Two were not worth a commit: - -- **Adding this branch's own review-notes file to the sdist inventory in `wip/pr-14-review-notes.md`.** That listing is `tar -tzf` output captured from a build made before the notes file existed. Extending it changes nothing about the finding it supports — that the sdist ships internal planning documents — or about what someone picking the item up would do. -- **The counts in the completion narrative above ("two page-ceiling tests", "two hardcoded counts").** The no-hardcoded-counts rule exists because a live inventory drifts: it "creates diff churn, goes stale silently, and adds no value". None of that applies to a closed record of a finished review round, which gains no members and cannot go stale. The rule targets counting things that are still moving. - -## Known gaps left open on purpose - -- No e2e suite exists in this repo (`tests/` holds only `unit/`), so the live `views` gate and the live paging envelope are pinned only by mocked bodies here; the JS suite pins both live. Adding an e2e suite is separate work. -- The remaining `@pipelex/sdk` surfaces without a Python counterpart (`lint`, `format`, `resolve`, `codegen`, `build_output` / `build_runner` / `concept` / `pipe_spec`, `run_codegen_check`, `get_method_closure`) are untouched by the cited releases and stay deferred; Phase 3 makes `docs/architecture.md` say so. -- The protocol-argument type guards (`pipe_code`, `mthds_contents`) belong in `mthds-python` and arrive here with the `mthds` floor bump once that package ships its Phase 1. diff --git a/docs/architecture.md b/docs/architecture.md index 390dd01..2e43f90 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -270,7 +270,7 @@ The download twin of input preparation, and the Python twin of `@pipelex/sdk`'s ## Out of scope -- The `/v1/build/*` helpers — `build_output`, `build_runner`, `concept`, `pipe_spec`. `build_inputs` shipped in 0.5.0 and was removed again once `prepare_inputs`, its only caller, moved onto `validate` + the input-form descriptor: this SDK no longer touches `/v1/build/*`, which the workspace is retiring (`wip/build-retirement/`). +- The `/v1/build/*` helpers — `build_output`, `build_runner`, `concept`, `pipe_spec`. `build_inputs` shipped in 0.5.0 and was removed again once `prepare_inputs`, its only caller, moved onto `validate` + the input-form descriptor: this SDK no longer touches `/v1/build/*`, which the workspace is retiring (L-260829-848001 in the workspace ledger). - Organization *switch* (a WorkOS session operation, not a `/v1` route). - A `~/.pipelex/config` file reader (env-only for now, matching the JS SDK). - A synchronous client facade. diff --git a/pipelex_sdk/crate_models.py b/pipelex_sdk/crate_models.py index 167ab4c..8712fe6 100644 --- a/pipelex_sdk/crate_models.py +++ b/pipelex_sdk/crate_models.py @@ -5,7 +5,7 @@ `CrateRequestBase` and `CrateInvalidReport` used to sit in a `build_models` module beside the `/v1/build/inputs` wire models; those went when `prepare_inputs` moved its signature source to the input-form descriptor and this SDK stopped calling `/v1/build/*` (workspace campaign -`wip/build-retirement/`). Nothing about the envelope changed in the move. +L-260829-848001). Nothing about the envelope changed in the move. `/v1/resolve` emits the normalized library crate, `/v1/codegen` projects that crate into stamped typed artifacts plus their lock. Both are Pipelex API extensions (NOT MTHDS Protocol routes) over diff --git a/tests/unit/test_prepare_inputs.py b/tests/unit/test_prepare_inputs.py index 69fcb4f..e8402d7 100644 --- a/tests/unit/test_prepare_inputs.py +++ b/tests/unit/test_prepare_inputs.py @@ -1,6 +1,6 @@ """`prepare_inputs` — signature-driven input preparation over the input-form descriptor. -Cases derive from the shared behavior matrix (`wip/upload/behavior-matrix.md`) and port +Cases derive from the behavior this SDK shares with `@pipelex/sdk` (`docs/input-preparation.md`) and port `pipelex-sdk-js/tests/prepare-inputs.test.ts`: file-bearing positions come from the DESCRIPTOR's declared kind (`document` / `image`), assets are uploaded and rewritten to `pipelex-storage://` in `url`, http(s)/storage references pass through, dedup keys on source identity, and the call diff --git a/wip/input-form-typed-narrowing.md b/wip/input-form-typed-narrowing.md deleted file mode 100644 index 1d4477a..0000000 --- a/wip/input-form-typed-narrowing.md +++ /dev/null @@ -1,44 +0,0 @@ -# Typing the descriptor and the contracts by import (`mthds.protocol`) - -This is this repo's tracker for **Stage 3.4** of the workspace input-form program (`../../wip/input-form/plan.md`), carried by ledger item `L-260826-c9b76b`. The program plan holds the *why* and the sequence; this file holds what the change is here, the decisions taken while making it, and what a reader needs to know afterwards. - -## The instruction - -Stage 3.4 applies decision **D-1**: the wire types of the input-form descriptor and of the pipe I/O contracts belong to the standard's clients — `mthds/protocol` in TypeScript, `mthds.protocol` in Python. Every SDK therefore narrows its opaque field **by import** rather than by restating the shape. Here that means two fields of `PipelexValidationReport` stop being bare mappings and start being the standard's own models, and the `mthds` floor moves to the version that publishes them. - -## What this retires, and why it is not a reversal - -The first program ruled (its D4) that `input_form` stays opaque, and the reason it gave was ownership plus drift: the descriptor vocabulary is owned elsewhere, so a second copy inside this SDK would be free to drift from it. That reasoning was sound and its conclusion is now obsolete, because the premise changed. When D4 was taken, no published Python package declared the descriptor, so "type it here" could only mean "restate it here" — a copy, and therefore drift. Since `mthds` 0.9.0 the standard's own client declares both artifacts, so typing them here means importing them: one declaration per language, nothing to drift from. D-1 supersedes D4 on that basis, and the principle D4 was protecting — this SDK is transport and does not own these types — is exactly what an import preserves and a restatement would have broken. - -## Where the boundary now sits - -Two fields of the valid arm are typed by import: `pipe_io_contracts: PipeIOContracts` (from `mthds.protocol.pipe_io_contracts`) and `input_form: InputForm | None` (from `mthds.protocol.input_form`). Two remain opaque, and for the reason that used to cover all four: `bundle_blueprint` and `graph_spec` have no published declaration to import, so a type here could only be a copy. When one of them gets a standard page and a client model, it moves the same way. - -The types are imported and used, never re-exported from `pipelex_sdk`. Re-exporting them would put this package's name on a vocabulary it does not own and would give consumers a second import path to drift against; a consumer that wants to name a node's type imports it from `mthds.protocol.input_form` directly, which is also how it reaches the per-kind models for narrowing. - -## Strictness: closed artifacts inside an open envelope - -This is the one thing worth getting exactly right, because getting it wrong turns a strictness improvement into a regression. - -The standard's models are **closed** shapes (`extra="forbid"`, decision D-5): a member this version of `mthds` does not define is version drift and fails the parse. The validate report itself is **extension-open** per the protocol's extension policy, and stays that way — `PipelexValidationReport` inherits `model_config = ConfigDict(extra="allow")` from `mthds`'s `ValidationReport`, and declaring two typed fields on a subclass does not touch that config. So the two closures compose the way the standard intends and nest rather than spread: - -- An unrelated field the server adds to the **report** — a new artifact, a cost estimate, another opt-in view — still parses and still rides `model_extra`, exactly as before this change. The `input-form-does-not-close-the-report` test pins that, and it is the regression guard against a future edit that reaches for `extra="forbid"` on the envelope. -- An undefined member **inside** a contract or a field descriptor now fails the parse. That is deliberate, it is the standard's own rule rather than this SDK's invention, and it is scoped to the artifact. - -## The break - -A valid report whose `pipe_io_contracts` predates the reshape — an input contract carrying the boolean `optional` instead of `presence`, or missing `multiplicity` / `item_count` — no longer parses, where before it rode through untyped. The hosted plane emits the reshaped contracts, so this is a break against runners older than that reshape and not against the API this SDK targets. No compatibility shim: an artifact that does not conform to the version of the standard this package pins is version drift, and reporting it at the parse is the whole point of D-5. - -## Checklist - -- [x] `mthds` floor moved to `>=0.9.0` in `pyproject.toml`, lockfile refreshed. -- [x] `pipe_io_contracts` and `input_form` typed by import in `pipelex_sdk/validation_models.py`, with the module docstring stating which members are typed by import, which stay opaque, and why. -- [x] Wire fixtures in `tests/unit/test_validation_contract.py` updated to conformant payloads — they were written for the opaque era and state neither the reshaped contract members nor the descriptor's pipe-slot facts. -- [x] Tests: the artifacts read as typed members; the report stays extension-open around them; drift inside an artifact and a violated cross-field invariant both fail the parse; a pre-reshape contract no longer parses. -- [x] `docs/architecture.md` — the validate section says what is typed and what stays opaque, and the paragraph that told a reader to go read the spellings out of an opaque mapping is retired. -- [x] `README.md` — the import map names where the descriptor and contract types come from. -- [x] `CHANGELOG.md` under `## [Unreleased]`. - -## Release - -None from this item. Decision **D-7** of the program plan: every Stage 3 repo lands on `dev` and records its warrant under `## [Unreleased]`; the versions are cut together at the plan's release cascade (item 4.0), when Stage 4 needs published artifacts. Where the plan or the ledger item says "minor bump" for this item, that means the changelog warrant, not a version cut. diff --git a/wip/method-source-adapter/review-deferrals.md b/wip/method-source-adapter/review-deferrals.md deleted file mode 100644 index cbd0399..0000000 --- a/wip/method-source-adapter/review-deferrals.md +++ /dev/null @@ -1,34 +0,0 @@ ---- -status: active -item: L-260907-28adb9 ---- - -# Deferred review findings — `method_source_to_contents` and the catalog decoder - -What review rounds on `pipelex-sdk-python#31` confirmed but did not fix, with enough detail to pick each up cold. Everything here was verified against the code; nothing rests on a reviewer's word alone. Findings owned by another repo are not here — they are ledger items (`L-260913-6ec559`, `L-260913-37ca47`). - -## The `RecursionError` conversion diagnoses the wrong cause when the caller's stack is deep - -`pipelex_sdk/product_models.py`, `_decode_method_source`. - -`json.loads` raises `RecursionError` for two different reasons that are indistinguishable at the point of the catch: the *source* is nested past what the decoder can descend, or the *caller* was already near the recursion limit when it called. The conversion reports both as "Method file source is nested too deeply to decode", which is a false statement about the data in the second case, and `method_source_to_contents` then reads a perfectly valid catalog array as a raw bundle and returns it without an error. - -Reproduced at the default recursion limit of 1000: a recursive caller that reaches depth 995 and then passes the valid catalog string `[{"name": "a.py", "content": "x = 1"}]` gets `ValueError("Method file source is nested too deeply to decode…")` from `parse_method_files`, and through `method_source_to_contents` gets `['[{"name": "a.py", "content": "x = 1"}]']` instead of `['x = 1']`. At depth 990 both behave correctly; at 998 a bare `RecursionError` escapes from the frame setup before `json.loads` is even reached, so the "never raises" contract has a floor no function in Python can lift. - -Deferred because the precondition is a program already within a handful of frames of the recursion limit, where essentially nothing behaves. It is recorded rather than fixed because the two causes genuinely cannot be told apart from inside the handler, so any "fix" is either a guard against an untestable state or a reworded message. If it is ever picked up, the honest change is the message — say what was observed ("the decoder ran out of stack") rather than what was inferred about the payload. - -## The integer digit-cap `ValueError` bypasses both curated handlers - -`pipelex_sdk/product_models.py`, `_decode_method_source`. - -`json.loads` on an integer literal past CPython's 4300-digit conversion cap raises a bare `ValueError` ("Exceeds the limit (4300 digits) for integer string conversion") which is neither a `JSONDecodeError` nor a `RecursionError`, so it passes through both `except` clauses untouched and reaches the caller with CPython's message and no `__cause__` chain instead of one naming the expected shape. - -The type contract still holds — it *is* a `ValueError`, which is what the docstring promises and what pydantic converts — and behaviour matches the platform, whose own `except ValueError` catches it identically, so `method_source_to_contents` reads it as a bundle on both sides. Deferred as message quality rather than correctness. Worth noting that its `RecursionError` sibling got an owner in the same commit while this one did not, which is the only reason it looks like an oversight. - -## The tests monkeypatch stdlib `json.loads` process-wide - -`tests/unit/test_method_files.py` and `tests/unit/test_method_source.py`, the deep-nesting tests. - -`product_models.json` *is* the stdlib `json` module object, so `mocker.patch.object(product_models.json, "loads", …)` replaces the attribute on the shared module rather than on a seam local to the module under test. Anything else running in the same process during those tests would get the fake. It is harmless here — pydantic-core is Rust and never routes through `json.loads`, and pytest-mock restores the attribute at teardown — and it is the only way to make the real decoder fail without pinning a nesting depth that is an interpreter build constant. - -Deferred as test hygiene. A module-local seam is available if it is ever wanted: replace the module reference in the module's own namespace (`mocker.patch.object(product_models, "json", …)` with a stub exposing `loads` and `JSONDecodeError`) rather than the attribute on the shared module. diff --git a/wip/pr-11-review-notes.md b/wip/pr-11-review-notes.md deleted file mode 100644 index a318f66..0000000 --- a/wip/pr-11-review-notes.md +++ /dev/null @@ -1,87 +0,0 @@ -# PR #11 — deferred review-agent findings - -Follow-up notes from triaging the SWE-bot review comments on [PR #11](https://github.com/Pipelex/pipelex-sdk-python/pull/11) (Release v0.5.0). Each item below was verified read-only against the code and against the two repos this SDK is contractually bound to — `../pipelex-sdk-js/` (`@pipelex/sdk`, the parity counterpart) and `../pipelex/` (the runtime whose `input_normalizer` the walk mirrors). - -The other flagged comment on the PR was a false positive and needed no follow-up (recorded under "Dismissed" below). - ---- - -## 1. Nested asset under a structured `url` field is not uploaded (confirmed, deferred) - -**Reported by:** codex — thread on `pipelex_sdk/prepare_inputs.py:153`. - -**Status:** Confirmed latent bug. Deferred because a correct fix is a cross-repo contract decision, not a local patch, and the SDK is at exact parity with `@pipelex/sdk`. - -### The bug - -`_resolve_node` classifies a template node as file content purely by shape: - -```python -def _is_file_content(node: Any) -> bool: - return isinstance(node, dict) and "url" in node -``` - -For a structured concept that has a top-level field literally named `url` **and** a sibling file-bearing field — e.g. `Article { url: str, cover: Image }` — the explicit template renders as: - -```json -{"url": "https://mock.invalid/url", "cover": {"url": "https://mock.invalid/url"}} -``` - -(The template generator special-cases any field named `url` or `*_url` at `../pipelex/pipelex/core/concepts/concept_representation_generator.py:314-320`, so `url` is not reserved to Image/Document.) - -`_is_file_content` then fires on the **top-level** `url` (`prepare_inputs.py:153`), so `_resolve_file_position` resolves only the article's text URL and returns early. The walk never recurses into `cover`, so a caller value like `{"url": "https://example.com/a", "cover": }` leaves the `cover` bytes **unuploaded** — the hosted run receives raw bytes at a nested Image position. - -### Why the runtime gets this right and the SDK does not - -The runtime `../pipelex/pipelex/pipeline/input_normalizer.py:61-93` dispatches on the Python **type** of the value: `isinstance(value, (ImageContent, DocumentContent))` vs `isinstance(value, StructuredContent)` (which recurses every field). It never keys on the presence of a `url` dict key. The SDK's shape-only heuristic is a documented approximation of that type-based classifier (`prepare_inputs.py:7-12`, `docs/input-preparation.md`); the two coincide for the common case and diverge exactly on this shape. - -### Why it is not cleanly fixable at the SDK layer - -- **No type info at nested positions.** Only the top-level template envelope carries a `"concept"` key, and it is dropped when the walk reads `entry["content"]` (`prepare_inputs.py:205`). At a nested `{"url": ...}` the SDK has nothing but shape to go on — it cannot tell an `ImageContent` from a structured concept whose single field is `url: str`. -- **Shape refinement is ambiguous.** A single-key `{"url": str}` structured concept is indistinguishable from single-key image content; and multi-key canonical image content is deliberately supported (the existing test `tests/unit/test_prepare_inputs.py:136` feeds a two-key `{"url", "mime_type"}` cover). So neither "single-key only" nor "keys ⊆ image/document vocabulary" is reliable without hardcoding the runtime's field vocabulary into the SDK — fragile and still collision-prone. -- **Parity constraint.** The Python code is a faithful port of `../pipelex-sdk-js/src/prepare-inputs.ts:67-69,172-174` (identical `isFileContent` + identical short-circuit). Any behavioral change must land in both SDKs in the same coordinated change; a Python-only fix would break the parity invariant. - -### Recommended real fix (upstream, coordinated) - -Thread concept/type information into the **nested** positions of the explicit inputs template in `../pipelex/` (so a nested node self-identifies as Image/Document vs structured), then update both SDKs to classify by that tag instead of by the `url` key. That is a template-contract change and should be decided with the runtime + JS SDK owners together. - -### Fragile interim mitigation (only if forced, must be mirrored in JS) - -Make `_is_file_content` treat a dict as file content only when `"url" in node` **and** every other key is drawn from the known Image/Document optional-field set (`public_url, mime_type, filename, title, snippet, caption, width, height, source_prompt, source_negative_prompt`). This recovers the `{url, cover}` case while preserving the multi-key image case in the current tests. It does **not** fix the single-key `{url: str}` structured concept (still shape-indistinguishable), so it is a partial mitigation, not a fix — which is why the honest call is to defer and raise the contract question upstream. - -### Repro (documentation only — not added to the suite) - -A failing test would break release CI, so this is recorded here rather than committed. In `tests/unit/test_prepare_inputs.py` style (`_FakePrepareClient`, `asyncio.run`, single `TestPrepareInputs` class): - -- template entry: `_entry("demo.Article", {"url": "https://mock.invalid/url", "cover": {"url": "https://mock/c.png"}})` -- inputs: `{"article": {"url": "https://example.com/a", "cover": bytes([7, 7])}}` -- expected once fixed: `cover` rewritten to a `pipelex-storage://` url, top-level `url` passed through, `len(uploads) == 1`. - -Under today's code this asserts 0 uploads and the cover bytes leak — cleanly documenting the gap. - ---- - -## 2. Oversized upload surfaces as `UploadTransportError`, not `RejectedAssetError` (needs-judgment, server-side) - -**Reported by:** greptile (P1) — thread on `pipelex_sdk/upload.py:101-103`. The literal comment ("400/422 should be `RejectedAssetError`") is a **false positive for this PR** (see below), but verification surfaced a real cross-repo seam worth a decision. - -### Why the literal comment is a false positive - -`_map_upload_error` (`upload.py:90-105`) maps `413 → RejectedAssetError`, `401|403 → UploadAuthenticationError`, `404 → UnsupportedUploadCapabilityError`, and everything else → `UploadTransportError`. `../pipelex-sdk-js/src/upload.ts:196-241` is byte-for-byte identical (only 413 maps to a rejected asset). Mapping 400/422 → `RejectedAssetError` in Python alone would diverge from `@pipelex/sdk`, which is the repo's controlling invariant. So the flagged line is correct-by-design. - -### The real seam - -`pipelex-api` rejects an oversized upload with **422**, not 413: the base64 `data` field has a Pydantic `max_length=MAX_UPLOAD_BASE64_CHARS` constraint (`pipelex-api/api/routes/uploader.py`), which FastAPI turns into a 422 request-validation error (asserted by `pipelex-api/tests/unit/test_uploader.py`). The explicit `len(data) > MAX_UPLOAD_BYTES` → 413 path is only reachable in the narrow band where the char count passes but decoded bytes marginally exceed the cap. A base64-decode failure returns 400. - -Consequence: the documented "asset too big → `RejectedAssetError`" category is effectively **unreachable in the common case**, in *both* SDKs — the most common oversized rejection comes back as a transport error. - -### Options (pick one, coordinated) - -- **Preferred:** make `pipelex-api`'s size rejection surface as **413** (align the Pydantic-`max_length` rejection with the explicit 413 check) so the existing "413 == rejected asset" contract holds end-to-end. No SDK change; parity preserved. -- **Alternative:** treat 422 as a rejection at the SDK layer — extend `case 413:` → `case 413 | 422:` — but only if landed in **both** `pipelex_sdk/upload.py` and `../pipelex-sdk-js/src/upload.ts` together, with matching tests. `RejectedAssetError` carries `status`, so callers could still tell 413 from 422. - ---- - -## Dismissed (no follow-up needed) - -**Empty-list template skips uploads** — greptile (P2), `prepare_inputs.py:155-160`. Can't-happen: the explicit template never emits an empty list. Top-level multiplicity wraps the content in a one-element exemplar (`../pipelex/pipelex/core/concepts/concept.py:225-226`) and nested `list[T]` fields render one example item (`../pipelex/pipelex/core/concepts/concept_representation_generator.py:210-236`). Line 156's `template_node[0]` already relies on non-empty, and JS carries the identical `> 0` guard (`prepare-inputs.ts:175`). Recorded here so it is not re-flagged. diff --git a/wip/pr-14-review-notes.md b/wip/pr-14-review-notes.md deleted file mode 100644 index fbb3c0f..0000000 --- a/wip/pr-14-review-notes.md +++ /dev/null @@ -1,63 +0,0 @@ -# PR #14 — deferred review findings - -Findings from the pre-landing review of [PR #14](https://github.com/Pipelex/pipelex-sdk-python/pull/14) (`feature/Typed-method-id-run-option`, reviewed at `301d96e`). Each item below was verified against the code and, where the claim was about the built artifact, against a locally built sdist. None of them blocks landing the branch; each is deferred because acting on it reaches outside what this branch set out to change. - -The findings that *were* acted on during the review are recorded in `CHANGELOG.md` under `[Unreleased]`, not here. - ---- - -## 1. The published sdist carries the repo's internal planning documents - -**Status:** Confirmed, pre-existing, widened by this branch. Deferred because the fix is a packaging change to `pyproject.toml`, which this branch does not touch and which has release implications worth deciding on their own. - -`pyproject.toml` declares `build-backend = "hatchling.build"` (`pyproject.toml:40`) and carries no `[tool.hatch.build.targets.sdist]` section, so hatchling falls back to including everything the VCS does not ignore. An sdist built at the review point confirms it. The listing below is that build — version 0.5.0, at `301d96e` — and is kept as it was taken rather than re-run, so it predates this very file and is not the current release's archive: - -``` -$ uv build --sdist -$ tar -tzf dist/pipelex_sdk-0.5.0.tar.gz -pipelex_sdk-0.5.0/CLAUDE.md -pipelex_sdk-0.5.0/TODOS.md -pipelex_sdk-0.5.0/Makefile -pipelex_sdk-0.5.0/uv.lock -pipelex_sdk-0.5.0/docs/HANDOFF.md -pipelex_sdk-0.5.0/wip/pr-11-review-notes.md -pipelex_sdk-0.5.0/wip/updates.md -pipelex_sdk-0.5.0/tests/... -``` - -`CLAUDE.md`, `wip/pr-11-review-notes.md`, `docs/HANDOFF.md`, the `Makefile` and the whole `tests/` tree already shipped this way before the branch, so this is not a regression it introduced. What the branch adds is `TODOS.md` and `wip/updates.md` — the tracker and the design — which together are a substantial share of the archive and are addressed to reviewers of this PR rather than to anyone installing the package. `docs/HANDOFF.md` is a related case already on PyPI: it describes creating this repo from scratch and reads as rot to anyone who finds it in a release. - -Nothing breaks — an sdist is not what `pip install` normally consumes, and none of these files is importable — so this is about what a public package says about itself, not about correctness. - -**If picked up:** declare an explicit sdist include list (or an exclude list covering `wip/`, `TODOS.md`, `CLAUDE.md`, `Makefile` and `docs/HANDOFF.md`), decide deliberately whether `tests/` should stay (some consumers value a testable sdist), and land it with a release rather than inside a feature branch. - -## 2. The client class docstring dates its surfaces by build-plan phase - -**Status:** Confirmed, pre-existing. Deferred because Phase 2 of this branch scoped its citation sweep to bare workspace-private *paths*, and widening that scope mid-branch was a judgement call the tracker declined elsewhere for the same reason. - -`pipelex_sdk/client.py:176` and `pipelex_sdk/client.py:178` describe the run lifecycle as "(added in Phase 2)" and the product surface as "(added in Phase 3)". Those phase numbers refer to the original build plan for this package. They travel to PyPI in the class docstring of the one class every consumer instantiates, where they resolve to nothing — the same failure mode as the repo-relative spec paths Phase 2 replaced, in a different spelling. - -**If picked up:** replace each marker with what the reader actually needs (the release the surface shipped in, or nothing at all), and sweep for the same pattern elsewhere in the shipped modules. - -## 3. `start_and_wait` documents fewer exceptions than it propagates - -**Status:** Confirmed, pre-existing. Deferred as too small to justify widening this branch's diff. - -`pipelex_sdk/client.py` documents `Raises: RunFailedError` and `RunTimeoutError` on `start_and_wait`, but the method reaches `_merge_hosted_run_extensions` on both of its paths — the durable one through `start` and the fallback through `_execute_blocking` → `execute` — so it also propagates `PipelineRequestError` for a reserved key on `extra` and, since this branch, for a non-string `method_id`. The `Raises:` sections of `execute` and `start` were corrected during this review; `start_and_wait` was left alone because its omission predates the branch and is not about anything the branch changed. - -**If picked up:** add the `PipelineRequestError` line to `start_and_wait`, and while there check `wait_for_result` and the product methods for the same drift. - -## 4. `PipelineRun.pipe_statuses` is a contract no server fills, in three repos at once - -**Status:** Confirmed, pre-existing, and the most consequential item here. Deferred because it cannot be resolved inside this repo: removing the field locally would break the parity invariant this package is built on, and the decision belongs to whoever owns the run wire contract. - -`pipelex_sdk/product_models.py` declares `pipe_statuses: dict[str, PipeStatus] | None = None` on `PipelineRun`, with `PipeStatus` as its supporting enum. The platform never sends it. `RunPublic` — the model FastAPI serializes `GET /v1/runs` and `GET /v1/runs/{id}` through — declares no such field (`pipelex-server/shared/src/pipelex_shared/schemas/run.py:180`), and a response model strips whatever it does not declare. A `grep -rn "pipe_statuses"` over the entire `pipelex-server` monorepo returns nothing at all, so no route, worker or Lambda writes it either. The field therefore reads `None` on every run this SDK will ever parse, and a consumer branching on it gets a silently empty answer rather than an error. - -The same dead field exists in the two sibling repos, which is what makes it a workspace question rather than a local cleanup: - -- `pipelex-sdk-js/src/product-models.ts:322` — `pipe_statuses?: Record | null;`, with the enum at `:291`. This SDK is a port of that one, so dropping the field here alone would introduce exactly the parity gap `docs/architecture.md` → "Parity with `@pipelex/sdk`" exists to prevent. -- `pipelex-app/src/types/run.ts:24` — the same declaration, and `pipelex-app/src/components/method/run-history-list.tsx:269` renders a row of per-pipe progress dots gated on `{run.pipe_statuses && ...}`. Because the platform never sends it, that guard is always false and those dots have never appeared. Whether that is a missing feature or an abandoned one is the question to settle. - -So there are two coherent outcomes and this branch is the wrong place to choose between them: either the platform starts projecting per-pipe status onto `RunPublic` (and the webapp's dots light up), or the field is retired from all three clients together. - -**Filed:** `../wip/inbox/2026-08-25-workspace-pipe-statuses-dead-field-in-three-clients.md` (`to: workspace`, naming `pipelex-server/platform`, `pipelex-sdk-js` and `pipelex-app`). Whichever way the decision goes, this SDK follows the JS SDK; it should not move first. diff --git a/wip/pr-50-review-notes.md b/wip/pr-50-review-notes.md deleted file mode 100644 index e5b234b..0000000 --- a/wip/pr-50-review-notes.md +++ /dev/null @@ -1,18 +0,0 @@ ---- -status: active -item: L-260927-424b11 ---- - -# PR #50 — deferred review findings - -These are findings from the `/rev` passes on [PR #50](https://github.com/Pipelex/pipelex-sdk-python/pull/50) (`feature/Refused-start-raises-bare`). Round 1 reviewed `88805ea` and round 2 reviewed `9d397e3`. The findings that were acted on are in `CHANGELOG.md` under `[Unreleased]`. The findings that belong to another repo are ledger items: the copied message-reason helpers are L-260927-9d2df3 (mthds-python), and the JS twin's missing-route 404 is L-260927-578c32 (pipelex-sdk-js). This note keeps the one finding this repo owns and did not act on. - ---- - -## 1. A `200` from `/v1/version` whose body is not JSON escapes the handshake - -**Status:** Unverified (raised by cubic in round 2 and not put to a verifier). It predates this branch and is deferred at the round-2 bar as a defect that does not matter. - -`_supports_run_lifecycle` catches `(ApiResponseError, httpx.HTTPError, ValidationError)` around `self.version()`, and its comment says a body that is no version makes the client assume hosted. The inherited `version()` calls `response.json()` before it validates, though. A `200` whose body is not JSON at all, such as an HTML page from a proxy or a mistyped base URL, raises `json.JSONDecodeError`, which none of the three catches, so `start_and_wait` raises that instead of assuming hosted. `pydantic.ValidationError` and `json.JSONDecodeError` both subclass `ValueError`, so catching `ValueError` in place of `ValidationError` would make the comment true. - -It does not matter much in practice. Assuming hosted leads straight to `start`, which reads the same non-JSON `200` through `response.json()` and fails the same way, so the caller would see the same decode error one request later. The change is worth making if the handshake is ever touched again. diff --git a/wip/prepare-inputs-selectors/plan.md b/wip/prepare-inputs-selectors/plan.md deleted file mode 100644 index a8a6228..0000000 --- a/wip/prepare-inputs-selectors/plan.md +++ /dev/null @@ -1,58 +0,0 @@ ---- -status: landed -item: L-260829-8a25d5 ---- - -# `prepare_inputs`: three selectors, signature from the input-form descriptor - -The Python half of the workspace campaign retiring `/v1/build/*` (epic `L-260829-848001`, `wip/build-retirement/` at the workspace root). - -## The design of record is the JS one - -This repo writes no second design. `pipelex-sdk-js/wip/prepare-inputs-selectors/design.md` holds the investigation, Louis's ruling of 2026-08-29, the surface, the walk, the pipe-selection ladder, the error wordings, the alternatives rejected and the known limits — and it names this item's mandate explicitly: `prepare_inputs` lands the same surface, and because `build_inputs` and `BuildInputsRequest` exist here only to back it, this item deletes them. - -The JS twin (`L-260829-300c50`) landed as `pipelex-sdk-js` PR #42 (`bea4632`) and is the reference implementation. Divergence from it is a bug unless recorded below. - -## What this repo did - -- `prepare_inputs(client, *, files=None, method_ref=None, method_id=None, pipe_ref=None, inputs)` — keyword parameters rather than JS's `never`-pinned discriminated union, matching how `validate` already takes its selectors here. Empty-as-absent and the exactly-one check run before any request, raising `InputPreparationError`. -- One `validate(..., allow_signatures=True, views=["input_form"])` per call; the walk is a `match` over the descriptor's item classes rather than over `kind`, because each `*Field` derives from its `*Item` — one set of patterns covers the named layer (top level, `object.fields`) and the nameless one (`list.item`), and it narrows for pyright where matching on `node.kind` would not. -- `PipelexValidationReport.default_pipe_ref` added ahead of the server (`L-260829-0208c7`), as JS did. -- `build_inputs` and the `BuildInputs*` models deleted. `build_models.py` deleted with them: the three survivors it also held — `MthdsFileItem`, `CrateRequestBase`, `CrateInvalidReport` — moved to `crate_models.py`, beside the routes that still use them. -- The explicit `{concept, content}` envelope is now accepted. This was a **pre-existing parity gap**, not part of the item's letter: JS gained it in an earlier release and Python never did, so the two SDKs would not have been identical after the fix. Ruled in scope with the user on 2026-08-30. - -## Decisions taken here - -| Decision | Why | -|---|---| -| Keyword selectors, not a request model | The repo's own `validate` idiom; `architecture.md` already records the JS-vs-Python signature-shape divergence as idiomatic per language. | -| `build_inputs` deleted, where JS kept `buildInputs` | JS has a wrapper family (`buildOutput`, `buildRunner`, `concept`, `pipeSpec`) retiring together under `L-260829-eefc3f`. Python only ever had this one, added in 0.5.0 solely to back `prepare_inputs`. | -| `build_models.py` folded into `crate_models.py` | A module named for the build routes cannot go on owning the crate envelope after those routes leave. | -| A local `_non_empty_string`, not `client._normalized_selector` | That helper is private to the client boundary and raises `PipelineRequestError`; every failure of this module owes an `InputPreparationError`. | -| Two helpers, not one: `_caller_selector` beside `_non_empty_string` | Review round 1. The single lenient helper read both a CALLER's selector and the OPAQUE `bundle_blueprint`, and those want opposite answers for a non-string: absent for the payload whose schema is the runtime's, refused for the argument. Raising inside the shared helper — the suggested fix — would have made the defensive blueprint reads throw on a shape they exist to tolerate. | -| No fetch budget on the signature call | `validate` already rides the 20-minute ceiling; the 3-minute budget exists to *raise* the ~30s poll-ceiling routes. JS implemented this and reverted it — do not re-add. | - -## What this supersedes - -`wip/pr-11-review-notes.md` recorded a nested-file limitation of the old template walk: a top-level `url` key caused an early return, so a sibling file field went un-uploaded, and the note explained that shape refinement was ambiguous because the walk dropped the envelope's `concept`. The descriptor walk removes that class of problem structurally — position and kind are stated, never inferred — so the note is history, not open work. - -## Review - -Round 1 (2026-09-07) confirmed one defect in two threads and one wrong docstring, both fixed on the branch: - -- **A non-string selector was read as absent.** `_non_empty_string` coerced any non-string to `None`, so `method_ref=123` beside a real `files` passed the exactly-one check and prepared against a method the caller never named, and a non-string `pipe_ref` was absorbed by the pipe defaulting. Split into `_caller_selector` (refuses, per the decision row above) and the unchanged lenient reader, with `pipe_ref` normalization hoisted so both refusals land on the same pre-request boundary. -- **The `Raises:` section named the wrong exception.** `validate` is 200-diagnostic and stays on the inherited `httpx.HTTPStatusError` regime, not the product routes' `ApiResponseError`, so a caller following the docstring would have missed exactly the no-verdict failures it listed. - -Nothing else was raised. - -## Landing - -PR #21 merged to `dev` as `7b1892f`, closing `L-260829-8a25d5` (kind `fixed`). CI was green across every lint and test job on the reviewed commit; `make agent-check` and `make agent-test` both pass. The merge has not reached `main`, which is the deliberate part — see Release below. - -`L-260826-ddd843` was advanced, not closed: the Python half of the two misclassifications is fixed on `dev`, and that item's own bar is a shipped release from **both** SDKs, which neither has cut. - -The one thing this landing leaves open for a person: `L-260830-f5b65e` re-points `prepare_inputs` onto `POST /v1/input-form` before the Python release, and it waits on the pipelex-api route `L-260830-352005`. - -## Release - -None from this item directly. The change lands on `dev` and records its warrant under `## [Unreleased]`; `/release` cuts the version. `L-260826-ddd843` (the two misclassifications) closes only when **both** SDKs have shipped a release carrying the fix — the JS half was still unreleased when this landed. diff --git a/wip/updates.md b/wip/updates.md deleted file mode 100644 index b0013f1..0000000 --- a/wip/updates.md +++ /dev/null @@ -1,176 +0,0 @@ -# Updates warranted by pipelex-api 0.17.0 / 0.18.0, the pipelex-server bump, and `@pipelex/sdk` 0.14.0 - -**Status: implemented.** Every change designed below landed, and shipped in `pipelex-sdk` v0.6.0. [`TODOS.md`](../TODOS.md) is the implementation tracker for this design and carries the per-item status; read it for what was done, and this file for what was intended and why. This document is deliberately left as it was written, so its sections below still speak in the future tense and its §7 decisions stay quotable from the tracker. - -A design for what this repo still owes after the three sources named in the title, written on the `feature/Typed-method-id-run-option` branch, which already carries the typed `method_id` run option and the honest `delete_method` contract. Every claim below was checked against the code it names; line numbers are as of 2026-08-25 and will drift. - -## Verdict - -Yes — three groups of work, in decreasing order of what the cited releases actually ask for: - -1. **The `/v1/validate` surface moved, and this SDK has not followed.** pipelex-api 0.17.0 (via the `pipelex` 0.52.0 pin) added `warnings`, `liftable_pipes`, `input_form`, `missing_pipe_code` and `suggested_fix` to the report; 0.18.0 gated `input_form` behind a new `views` request list. `@pipelex/sdk` 0.14.0 mirrored all of it. Here, nothing crashes — every affected model is `extra="allow"`, so the new fields ride `model_extra` — but nothing is typed either, and there is no way to ask for the `input_form` view at all. This is the direct answer to the question and is purely additive. See §1. -2. **Two documentation corrections `@pipelex/sdk` 0.14.0 made apply verbatim here**: the `TokensUsageRecord` brand attribution, and citations of workspace-private paths from a public package. See §2. -3. **Found while checking, and more urgent than either: `list_methods` and `list_runs` crash against the deployed platform.** The platform reshaped both routes into `{items, next_cursor}` page envelopes (in prod since 2026-08-18 and 2026-08-11 respectively); this SDK still iterates a bare array, and its unit tests mock the old shape, which is why the suite is green. `PipelineRun` also declares `method_id` and `pipe_code` as required strings where the platform serves `str | None`. `@pipelex/sdk` fixed all of this in 0.10.0 / 0.11.0. This is a breaking fix and it is not optional. See §3. - -A fourth item is a decision already taken rather than a release to mirror: the 2026-08-25 boundary-validation decision recorded in `pipelex-sdk-js/wip/boundary-option-type-validation.md` names this repo for its Phase 2. See §4. - -What needs **no** change is listed in §5, so nobody re-derives it. The four design choices that were open in the first draft were decided on 2026-08-25 and are recorded in §7. - -## 1. The validate surface - -### 1.1 Where the Python SDK stands today - -- `PipelexValidationReport` (`pipelex_sdk/validation_models.py:84`) inherits `extra="allow"` from `mthds.protocol.models.ValidationReport`, so a 0.17+ body's `warnings`, `liftable_pipes` and `input_form` land in `model_extra`. Untyped, but parsed. -- `ValidationErrorItem` (`validation_models.py:62`) inherits `extra="allow"` from `ValidationDiagnostic`, so `missing_pipe_code` and `suggested_fix` land in `model_extra` the same way. -- Every optional member of `ValidationErrorItem` is already `T | None = None`. The JS 0.14.0 breaking change (`T` → `T | null`, forced by the valid arm serializing unset locators as explicit `null` inside `warnings[]`) therefore has **no Python counterpart** — pydantic reads both a dropped key and an explicit `null` into the same `None`. That asymmetry still deserves a regression test here, because it is the one thing a future "tighten to required" edit would break; the workspace inbox item `wip/inbox/2026-08-25-workspace-validation-error-item-spec-gaps.md` explicitly asks for the Python mirror to be checked on this point, and this section is the answer. -- The hosted `/v1/validate` is the platform proxying to the runner (`pipelex-server/platform/src/pipelex_platform/routers/v1/tooling_proxy.py`), so once `feature/Bump-pipelex` (which moves `api-hosted` to the `pipelex-api` `v0.18.0` tag and the core to `pipelex==0.52.0`) is deployed, `api.pipelex.com` serves exactly the contract a bare 0.18.0 runner serves today, `views` gate included. Nothing in that branch changes any other route this SDK calls; its only non-pin edits are the Temporal dry-validate activity carrying `input_form` through to the route that gates it. - -### 1.2 `views` — the structured-view opt-in on `validate` and `validate_files` - -Add `views: list[str] | None = None` to `PipelexAPIClient.validate` (`pipelex_sdk/client.py:449`) after `render`, and `views: list[str] | None = None` to `validate_files` (`client.py:491`) after `render`. In Python this is not the breaking change it was in TypeScript: the parameter is appended last and callers pass it by keyword, so no existing positional call moves. - -Semantics, mirroring `@pipelex/sdk` exactly: - -- `None` (the default) → the `views` key is **not sent**. This is the invariant that keeps an opt-in view opt-in: the default response stays byte-identical for the consumers that discard it (hook pipelines, CI gates, agent loops). -- A list → sent **verbatim**, including an explicitly empty `[]`. Unlike `render`, nothing is injected and nothing is de-duplicated: the server resolves tokens as a set and lenient-ignores unknown ones (never a `422`), so client-side normalization would only hide what the caller asked for. -- Today `input_form` is the only supported token; a constant for it belongs in `validation_models.py` next to the field it gates, not a closed enum on the parameter — the spec deliberately keeps the request boundary open so a stale token never fails a call. - -It reaches the wire through the base transport seam with no `mthds-python` change: `_post_validate` merges `extra` into the body as top-level keys, and its reserved set `_VALIDATE_REQUEST_ARGS` is only `{mthds_contents, allow_signatures}` (`mthds-python/mthds/runners/api/client.py:383`). `views`, like `render` and `mthds_sources` already, is a Pipelex-API carriage extension, so the protocol client stays unaware of it — the same layering as `method_id` on the run routes. - -### 1.3 The valid arm's new fields - -On `PipelexValidationReport`: - -- `warnings: list[ValidationErrorItem] = Field(default_factory=…)` — advisory lints on a **valid** bundle. Same item type as `validation_errors[]`, so one parser serves both channels, but they never flip `is_valid`. This is where the `hint_*` error types ride. -- `liftable_pipes: list[LiftablePipeEntry] = Field(default_factory=…)` — pipes the runtime may skip when an optional slot resolves absent. New model `LiftablePipeEntry(extra="allow")` with `pipe_ref: str`, `within_pipe_ref: str`, `skipped_when_absent: list[str]` (default empty — the server model defaults it too), `absence_source: str`, mirroring `pipelex/pipelex/pipeline/liftable_pipes.py`. -- `input_form: dict[str, Any] | None = None` — per-pipe input-form descriptors, keyed exactly like `pipe_io_contracts`. **Optional on purpose**: it is present only when the request named the `input_form` view (0.18.0), and a 0.17.0 runner emitted it unconditionally, so `None`-by-default is the one typing that reads a body from either runner correctly. Kept **opaque** like `bundle_blueprint`, `pipe_io_contracts` and `graph_spec`, for the same reason `@pipelex/sdk` keeps it opaque: the descriptor vocabulary is owned elsewhere (the runtime's `PipeInputFormDescriptor`, the `@pipelex/mthds-form` kernel, the `docs/specs/mthds-input-form-descriptor.md` contract), and a second copy here would be free to drift. - -Both list fields default to empty rather than being required, and that is a deliberate divergence from the runtime model, where they are always populated. This SDK is pointed at bare runners of whatever version a user runs; a pre-0.52 body with neither key must keep parsing, exactly as `bundle_blueprint` already defaults. The default is also what the runtime emits for a clean bundle, so no caller can tell the two apart — which is the point. - -`PipelexInvalidReport` gains nothing: the invalid arm never carries `warnings` or `input_form` (they derive from a crate that was never assembled), and the e2e evidence on the JS side pins `"warnings" not in report`. - -### 1.4 `ValidationErrorItem` additions and the fix vocabulary - -On `ValidationErrorItem`: `missing_pipe_code: str | None = None` (symmetrical with the existing `missing_concept_code`) and `suggested_fix: SuggestedFix | None = None`. - -New models in `validation_models.py`, mirroring `pipelex/pipelex/suggested_fix.py` and the OpenAPI artifact (`pipelex-api/docs/openapi/pipelex-api.openapi.yaml`, schemas `SuggestedFix`, `SetKeyOp` … `RemapValueOp`, `FixSafety`): - -- `FixSafety(StrEnum)`: `SAFE = "safe"`, `UNSAFE = "unsafe"`, with an `is_safe` property (house style: never compare enum values inline). -- `FixOpKind(StrEnum)`: the seven kinds — `set_key`, `ensure_table`, `delete_key`, `delete_table`, `rename_table_key`, `move_key`, `remap_value`. -- `TomlScalar: TypeAlias = str | int | float | bool` and `TomlValue: TypeAlias = TomlScalar | dict[str, TomlScalar]` — what a `set_key` writes; deeper nesting is not modelled because the server does not emit it. -- One model per op, each `kind: Literal[FixOpKind.X]` and exactly its own members: `SetKeyOp(key, value)`, `EnsureTableOp()`, `DeleteKeyOp(key)`, `DeleteTableOp()`, `RenameTableKeyOp(key, new_key)`, `MoveKeyOp(key, new_table_path, new_key)`, `RemapValueOp(key, mapping: dict[str, str])`. All carry `table_path: list[str]` (empty for the document root; the OpenAPI artifact marks it `minItems: 1` on `ensure_table` / `delete_table`, which is worth a `Field(min_length=1)` since it costs nothing and mirrors the artifact). -- `FixOp: TypeAlias = Annotated[SetKeyOp | … | RemapValueOp, Field(discriminator="kind")]` — narrowing is `match op: case SetKeyOp(): …`, which is the Python spelling of the JS `kind` narrowing. -- `SuggestedFix(extra="allow")`: `fix_code: str`, `description: str`, `safety: FixSafety`, `source: str | None = None`, `ops: list[FixOp]`. - -Two decisions inside that mirror: - -- **Reader models, not runtime models.** The runtime declares these `frozen`, `extra="forbid"`, with wildcard-refusing validators, because it *plans* fixes. This SDK only reads them, so the ops follow the SDK's response-model convention (`extra="allow"`) and carry none of the validators. A new server-side member on an op must not break parsing here. The two runtime invariants a type cannot carry (`*` is the wildcard segment, refused as a `key` on every kind but `remap_value`; `ensure_table` / `delete_table` need a non-empty `table_path`) go in docstrings, as the JS mirror did. -- **The `kind` vocabulary is closed, and an unknown kind raises.** A pydantic discriminated union needs `Literal` tags, so a kind this SDK does not know fails the parse of the whole verdict. That is consistent with how `ValidationErrorCategory` already behaves (pinned by `test_unknown_category_is_rejected`): the vocabulary is a closed `StrEnum` upstream, a new kind is a `pipelex` release this SDK mirrors, and a loud failure beats a silently unnarrowable op. The alternative — a catch-all op with `kind: str` through a callable `Discriminator` — is more machinery for a repair proposal that is advisory in the first place; noted in §7 as the one place a reviewer might reasonably disagree. - -`error_type` stays `str | None` — an open string, as in the JS mirror. The 0.17.0 changelog notes the union gained the advisory `HintLintErrorType` members; typing it as a closed enum here would turn every runtime enum addition into an SDK break for no consumer benefit. - -Naming stays neutral (`SuggestedFix`, `FixOp`, `warnings`, `liftable_pipes`, `input_form`): fixes and lints are language-level concepts, and the runtime names them brand-neutrally too. The `Pipelex` prefix stays on the two envelope types only. - -### 1.5 Tests - -- `tests/unit/test_client_validate.py`: `views` sent verbatim when given; the key absent from the body when `None`; an explicit `[]` sent as `[]`; `validate_files` threads `views` through; `render` behaviour unchanged alongside it. -- `tests/unit/test_validation_contract.py`: a valid body carrying `warnings`, `liftable_pipes` and `input_form` parses into typed fields, with `input_form` keyed like `pipe_io_contracts`; the JS null-bearing warning fixture (`pipelex-sdk-js/tests/client.test.ts`, "carries advisory warnings on the VALID arm, with the valid arm's explicit nulls") parses with every explicit `null` reading as `None`; the existing pre-0.52 `VALID_BODY` still parses with both lists empty and `input_form` `None`; an invalid body carrying `missing_pipe_code` and a two-op `suggested_fix` parses, with `match`-narrowing reaching each op's own members; an unknown `kind` raises `ValidationError`; `FixSafety` / `FixOpKind` value sets are the locked vocabularies. -- The JS suite also pins the gate **live** (`tests/e2e/tools.e2e.ts`: absent by default, present when asked, unknown token lenient). This repo has no e2e suite at all (`tests/` holds only `unit/`), so that half is not reproducible here today. Not a blocker for this change; recorded in §6 as a known gap rather than silently skipped. - -### 1.6 What stays opaque in the 0.17.0 contract move - -The 0.17.0 changelog lists more `/v1/validate` movements than the ones above, and none of them reaches a typed field here: `PipeInputContract.optional` → `presence`, the `fixed` multiplicity with `item_count`, and the widened `inputs` map all live inside `pipe_io_contracts` / `bundle_blueprint`, which this SDK carries as `dict[str, Any]` on purpose. A consumer that reads those dicts should know the new spellings; the SDK's docs (`docs/architecture.md`, validate section) should name them in one sentence so nobody discovers `presence` by surprise, but no model changes. - -## 2. Already on this branch, and the two documentation corrections still owed - -**Done here, matching `@pipelex/sdk` 0.14.0 field-for-field** (commit `cdd8793`): `method_id` as a typed keyword on `execute` / `start` / `start_and_wait`; the run-source precondition satisfied by a `method_id`-only body; `extra` rejecting `method_id` (`_HOSTED_RUN_ARGS`, `client.py:139`); an empty string treated as absent; the selector forwarded on the blocking fallback; `delete_method` returning `MethodDeletionAccepted`. `tests/unit/test_client_method_id.py` pins every one of the JS cases. Nothing further is owed on those. - -**Still owed** — the two prose fixes 0.14.0 shipped under "Fixed", which apply here for the same reason (`pipelex-sdk` is a public PyPI package): - -- **`TokensUsageRecord` attribution.** `pipelex_sdk/runs.py:23` and `:128`, `docs/run-usage.md:5` and `docs/architecture.md:103` say the record is "specified in the MTHDS protocol spec". It is not: inference accounting is a Pipelex runtime extension the MTHDS Protocol does not model, and the hosted API is what pins the wire contract. Reword as the JS mirror did (`pipelex-sdk-js/src/runs.ts`, `docs/architecture.md`). -- **Citations a reader cannot open.** `client.py:138` (`docs/specs/pipelex-platform-api.md`), `validation_models.py:44` (`conformance/conformance/validation_contract.py`), and the test-module docstrings at `tests/unit/test_validation_contract.py:4-5` / `:167`, `tests/unit/test_runs.py:12`, `tests/unit/test_client_method_id.py:4`, plus `docs/architecture.md:84`. Each names a workspace-private path by bare relative reference, which resolves to nothing for anyone who clones this repo and reads as rot. Replace each with the rule it was citing (the layered extension policy; the locked category vocabulary; the shared conformance corpus), as 0.14.0 did. No behaviour change. - -## 3. Found while checking: the product list routes are broken against the deployed platform - -### 3.1 Evidence - -- The platform serves `GET /v1/methods` as `MethodPage` — `{items: MethodSummary[], next_cursor: str | None}` — since `pipelex-server` commit `f4f8764` (2026-08-18, "paginate the method list, which was silently truncating"), and `GET /v1/runs?method_id=` as `RunPage` — `{items: RunPublic[], next_cursor}` — since `2c4e980` (2026-08-11). Both are ancestors of the latest `deploy(prod)` commit (`b9f9555`), so this is what `api.pipelex.com` answers today. Models: `pipelex-server/shared/src/pipelex_shared/schemas/method.py:265-309`, `schemas/run.py:180-236`. -- `list_methods` (`pipelex_sdk/client.py:767`) does `[MethodData.model_validate(item) for item in result]` over the JSON body. Iterating the envelope dict yields its **keys**, so the first call is `MethodData.model_validate("items")` → `pydantic.ValidationError` on every invocation. `list_runs` (`client.py:946`) fails identically. -- The unit tests mock the pre-paging bare arrays (`tests/unit/test_client_product.py:85`, `:379`), which is why nothing is red. -- `PipelineRun` (`product_models.py:369-370`) declares `method_id: str` and `pipe_code: str`; the platform's `RunPublic` declares both `str | None = None`, and both are genuinely null in practice (an ad-hoc run from an inline bundle; a pipe resolved from `main_pipe`). Once the envelope is fixed, the first such row raises. -- `@pipelex/sdk` took all three in 0.10.0 (`listRuns` → `RunPage`, `iterateRuns`, `getRunDetail`, nullable `PipelineRun` fields) and 0.11.0 (`listMethods` → `MethodPage`, `iterateMethods`, `MethodSummary`). `docs/architecture.md` here still claims full parity ("surface-complete, with no silent gaps"), which has been false since 2026-08-11. - -### 3.2 Design - -Breaking, and mirroring the JS shapes — with the wire kept snake_case, so the envelope field is `next_cursor` here where JS renamed it `nextCursor` for its own consumers. - -**Models (`product_models.py`):** - -- `MethodSummary(extra="allow")`: `method_id`, `name`, `description: str | None = None`, `created_at`, `deletion_state: MethodDeletionState | None = None`. Deliberately not a `MethodData`: no `mthds`, no `python`, no `updated_at`, because none is in the index projection and putting `mthds` back is what restored the truncation bug. -- `MethodPage(extra="allow")`: `items: list[MethodSummary]`, `next_cursor: str | None = None`. No total, by design. -- `RunPage(extra="allow")`: `items: list[PipelineRun]`, `next_cursor: str | None = None`. -- `RunErrorReport(extra="allow")`: `message: str | None = None`, `error_type: str | None = None` — the two fields a consumer may rely on out of the runner's verbose report. -- `PipelineRun`: `method_id: str | None = None`, `pipe_code: str | None = None`; add `org_id: str | None = None`, `created_by_user_id: str | None = None`, `error: RunErrorReport | None = None`. `pipe_statuses` stays as it is (the JS model keeps it optional; the platform's `RunPublic` no longer declares it, and `extra="allow"` covers either way). -- `RunDetail(PipelineRun)`: `mthds_contents: list[str] | None = None`, `inputs: dict[str, Any] | None = None` — the only read that carries what the run actually executed. -- `MethodData`: add `org_id: str`, `created_by_user_id: str` (required on the platform's `MethodPublic` and in the JS model), `description: str | None = None`, `deletion_state: MethodDeletionState | None = None`, and `python: list[MethodFile] = Field(default_factory=list)`. See the `python` decision below. -- `MethodWriteInput`: add `python: list[MethodFile] | None = None`. Because the write body is dumped with `exclude_none=True`, the platform's three-way contract falls out naturally: `None` → not sent → the stored Python is preserved; `[]` → serialized as `""` → clears it; a non-empty list → replaces it. Document that on the field. - -**Client (`client.py`):** - -- `list_methods(*, q: str | None = None, limit: int | None = None, cursor: str | None = None) -> MethodPage`. Query params are added on **presence** (`is not None`), never truthiness — an explicit empty `q` or cursor is bad input the API should reject, not something to silently drop into an unfiltered query that reads as working. Encode with `urllib.parse.urlencode` rather than string formatting; `q` is free text. -- `iterate_methods(*, q=None, limit=None) -> AsyncIterator[MethodSummary]` — an `async def` generator that follows the cursor. It must keep going **past empty pages** (`q` is a post-read filter over a bounded index slice per request, so `{items: [], next_cursor: "…"}` means "keep going"), stop on `next_cursor is None`, stop when the server hands back the cursor it was sent (checked before yielding, so rows are never double-counted), and **raise** rather than return past a runaway page ceiling set far beyond any real catalog. Deliberately not a `list_all_methods() -> list[…]`: an all-at-once helper needs a cap, and a cap means silently returning a truncated list — the exact bug paging removed. -- `list_runs(method_id: str, *, created_from: str | None = None, created_to: str | None = None, limit: int | None = None, cursor: str | None = None) -> RunPage`. `created_from` / `created_to` are instants (ISO-8601 with a UTC offset), not days; a naive timestamp is a platform `400`, surfaced as `ApiResponseError`. Same presence semantics. -- `iterate_runs(method_id, *, created_from=None, created_to=None, limit=None) -> AsyncIterator[PipelineRun]` — same loop, except an empty page **does** end it: the date bounds are index key conditions, so a run page is never empty-with-a-cursor. The difference is the server, not the client, and the docstring should say so. -- `get_run_detail(run_id: str) -> RunDetail` — `GET /v1/runs/{id}`, path-encoded like the other id routes. -- One thing to document that the JS mirror does not spell out: every `/v1/runs*` product route sits behind the platform's `require_surface_access()` gate (`pipelex-server/platform/src/pipelex_platform/deps.py:345`), which for API-key auth demands the `ff_api_keys` feature flag and fails closed with a `403`. That arrives here as an `ApiResponseError`, and a reader of `list_runs` should know a `403` means "flag", not "wrong key". - -**The `python` field.** On the wire `MethodPublic.python` is one string: the JSON text of a `[{name, content}]` array, or `""` for a method with no custom Python (`pipelex-server/shared/src/pipelex_shared/schemas/method.py:209`, `:251`), and the write side is the same string three ways (omitted → preserve, `""` → clear, text → replace). `@pipelex/sdk` never shows that string to callers: it exposes `MethodFile[]` and converts at the client boundary (`pipelex-sdk-js/src/client.ts:281` on read, `:292` on write) with `parseMethodFiles` / `serializeMethodFiles` from `mthds-js/src/protocol/method_files.ts`. That module is small — parse the JSON, check every entry is `{name: str, content: str}`, drop blank-content entries, and serialize an empty list as `""` rather than `"[]"` because `""` is the platform's clear sentinel. It lives in `mthds/protocol` on the JS side because `pipelex-mcp` consumes the same format and wanted one owner. - -The first draft of this document proposed exposing the raw wire string and asking `mthds-python` for the converter. That was over-engineered: with pydantic the whole converter is a `MethodFile` model plus a `TypeAdapter(list[MethodFile])` and the two sentinel rules, there is no second Python consumer that could drift, and the JS module's own docstring calls this the format "the hosted platform persists" — a Pipelex catalog concern, so this SDK is a proper home for it. **Decision: typed list, converter here.** `product_models.py` gains `MethodFile(name: str, content: str)` and a `parse_method_files(source: str | None) -> list[MethodFile]` / `serialize_method_files(files: list[MethodFile]) -> str` pair carrying the same rules as the JS pair (blank source or `"[]"` → `[]`; blank-content entries dropped on both directions; empty list → `""`). `MethodData` applies the parser through a `field_validator("python", mode="before")`, so `MethodData.model_validate(body)` keeps working unchanged at every call site; `MethodWriteInput` applies the serializer through a `field_serializer("python")`, so the write body still dumps with `exclude_none=True` and the three-way contract holds. Malformed wire text raises `ValueError` inside the validator and therefore surfaces as a `pydantic.ValidationError`, the same way any other malformed response body fails here. If the format ever gains a Python owner in `mthds-python`, this SDK adopts it then; nothing is filed to the inbox for it. - -`get_method_closure` (JS-only client-side sugar that parses the polymorphic `mthds` source into a run-ready closure) stays deferred: it is not moved by any of the cited releases, and the 0.5.0 changelog already recorded it as "deferred and additive" alongside `prepare_inputs`. - -### 3.3 Tests (`tests/unit/test_client_product.py`) - -Replace the two bare-array fixtures with envelopes and add: query encoding for `q` / `limit` / `cursor` and for `created_from` / `created_to`, including that an explicit empty string is forwarded rather than dropped; a null `pipe_code` / `method_id` row parsing; `get_run_detail` returning `mthds_contents` and `inputs`; `MethodData` carrying the new fields, with `python` parsed from the wire string into `MethodFile` entries and `""` reading as an empty list; `MethodWriteInput.python` three-way serialization (`None` absent, `[]` sent as `""`, a list sent as the JSON text); the `parse_method_files` / `serialize_method_files` pair round-tripping, dropping blank-content entries, and rejecting a non-array or a malformed entry. For the iterators, in a dedicated module (one `TestClass` per module): `iterate_methods` continues through an empty page with a live cursor and stops on `None`; both iterators stop on an unchanged cursor without re-yielding; `iterate_runs` stops on an empty page; `iterate_methods` raises past the ceiling. - -## 4. Boundary type validation for `method_id` (decision of 2026-08-25) - -`pipelex-sdk-js/wip/boundary-option-type-validation.md` records the decision, taken by Louis, that a published client validates request-option types at its boundary and throws `PipelineRequestError` rather than dropping or forwarding a wrong-typed value. Its evidence section cites this repo directly: `client.py:1003` is a bare `if method_id:`, which drops falsy non-strings (`0`, `[]`) and forwards truthy ones (`123`, `["mt_1"]`) to a server `422` — a *different* partition of wrong values than the JS client makes for the same argument on the same wire. The plan's Phase 2 names the fix: an explicit `is not None` presence check followed by an `isinstance(method_id, str)` check that raises, with `None` and `""` still normalizing to absent. - -The plan sequences the SDKs after the protocol packages so both inherit one behaviour for the protocol-level arguments. That ordering matters for `pipe_code` / `mthds_contents`, whose guards belong in `mthds-python`; it does not constrain `method_id`, which this layer owns outright and whose guard touches only `_merge_hosted_run_extensions` (`client.py:975`). **Recommendation: land the `method_id` guard in this update** — it is a few lines, this branch is already the `method_id` branch, and it closes the repo-specific finding in the JS wip doc — and take the protocol-argument guards later with the `mthds` floor bump once `mthds-python` ships its Phase 1. Decided 2026-08-25: it lands now (§7). - -## 5. Checked, no change needed - -- **pipelex-api 0.17.0's source-less `422` naming unhandled keys** (the `method_id`-at-a-bare-runner diagnosis): this SDK already forwards `method_id` on the blocking fallback precisely so that message reaches the caller; the `execute` docstring already describes it. -- **`storage_scope` / `callback_urls` / `orchestration_mode`** (0.15.0–0.16.0): layer-2 fields the hosted platform sends to the runner; not caller-facing and not an SDK concern. -- **The four OpenAPI schemas that went opaque, the `RunMetadata` split, the `.pipelex/` config schema, and the two authoring changes** (`required = true` + `default_value` rejected; unknown structure-field keys rejected): server-side and inside opaque dicts here; no wire field this SDK types moved. -- **`views` in `mthds-python`**: not needed. It is a Pipelex-API carriage extension exactly like `render`, and the base client's `extra` passthrough already carries it (§1.2). `mthds-js` likewise has no `views`. -- **Explicit-null locators** (JS 0.14.0's `T | null` widening): already `T | None = None` here (§1.1); only a regression test is owed. -- **`ValidationErrorItem.error_type` narrowing** to the new enum members: stays an open `str` by design (§1.4). -- **The 0.14.0 `validate()` positional break**: Python takes `views` by keyword after `render`; no positional call moves. -- **`method_id` typed option and `delete_method`**: already on this branch (§2). - -## 6. Change plan - -Four commits on this branch, each self-contained, each with its docs and changelog lines, each gated on `make agent-check` and `make agent-test`: - -1. **Validate surface** (§1): `validation_models.py` (new fields, `LiftablePipeEntry`, the fix vocabulary), `client.py` (`views` on `validate` / `validate_files`), the two test modules, `docs/architecture.md` (the validate section and the brand-boundary field list gain `warnings` / `liftable_pipes` / `input_form`, and a sentence on the opaque `presence` / `fixed` spellings), `README.md` quickstart mention of `views`. Changelog: **Added** (`views`; the typed valid-arm fields; `missing_pipe_code` / `suggested_fix` and the `SuggestedFix` / `FixOp` / `FixSafety` vocabulary), with a note that an older runner's body still parses. -2. **Prose corrections** (§2): the attribution and citation edits in `runs.py`, `validation_models.py`, `client.py`, the three test docstrings, `docs/run-usage.md`, `docs/architecture.md`. Changelog: **Fixed**, two entries mirroring 0.14.0's wording. -3. **Product paging and nullability** (§3): `product_models.py`, `client.py` (`list_methods`, `iterate_methods`, `list_runs`, `iterate_runs`, `get_run_detail`), `tests/unit/test_client_product.py` plus a new iterator test module, `docs/architecture.md` (product surface section rewritten for pages, the "Parity with `@pipelex/sdk`" section corrected — it must stop claiming surface-completeness and list the conscious exclusions honestly), `README.md` if it gains a listing example. Changelog: **Changed (breaking)** for the two return types and the nullable `PipelineRun` fields, **Added** for the iterators, `get_run_detail`, `MethodSummary` / `MethodPage` / `RunPage` / `RunDetail` / `RunErrorReport`, `MethodFile` with `parse_method_files` / `serialize_method_files`, the `MethodData` fields and `MethodWriteInput.python`, **Fixed** naming the crash. -4. **`method_id` type guard** (§4): `_merge_hosted_run_extensions`, one wrong-type parametrized test in `test_client_method_id.py`. Changelog: **Changed**. - -The version stays where it is under `## [Unreleased]` until `/release` cuts it; with the breaking items in commit 3 (and the ones already on the branch), that release is a minor bump. - -**Known gaps this plan leaves open, on purpose:** no e2e suite exists in this repo, so the live `views` gate and the live paging envelope are pinned only by mocked bodies here (the JS suite pins both live); the remaining `@pipelex/sdk` surfaces without a Python counterpart — `lint`, `format`, `resolve`, `codegen`, `build_output` / `build_runner` / `concept` / `pipe_spec`, `run_codegen_check`, `get_method_closure` — are unchanged by the cited releases and stay the conscious exclusions `docs/architecture.md` already records. - -## 7. Decisions (2026-08-25) - -The four questions the first draft left open, each answered by Louis on 2026-08-25 with the reasoning that settled it. - -1. **`input_form` stays opaque** — `dict[str, Any] | None = None`, matching the JS mirror and the ownership argument in §1.3. A `PipeInputFormDescriptor(fields: list[dict])` shell would type one level and still leave the field vocabulary opaque, which buys little. - - **Superseded, and by its own reasoning.** This answer assumed that typing the descriptor here meant declaring it here, which was true while no published Python package declared it — and a declaration here would have been a copy free to drift, exactly as the ownership argument said. `mthds` 0.9.0 changed the premise by publishing the artifact as a normative page with client models, so the field is now typed **by import** rather than restated, which keeps the ownership argument satisfied instead of overriding it. The same move applies to `pipe_io_contracts`. See [`input-form-typed-narrowing.md`](input-form-typed-narrowing.md). -2. **`MethodData.python` / `MethodWriteInput.python` are typed `list[MethodFile]`, with the converter in this repo.** The question was first posed as "raw wire string plus an inbox request to `mthds-python` for the parser", and the answer to "why do we need a parser at all?" dissolved that framing: the converter is a dozen lines of pydantic, the format is a Pipelex catalog concern rather than an MTHDS protocol one, and there is no second Python consumer to keep in step. Full design in §3.2; no inbox item is filed. -3. **The `method_id` type guard lands now**, in this update (§4). The protocol-argument guards for `pipe_code` / `mthds_contents` still wait for `mthds-python` to ship its Phase 1 and arrive here with the `mthds` floor bump. -4. **An unknown `FixOp.kind` raises** — closed `Literal` discriminator, `pydantic.ValidationError` on the whole verdict parse, consistent with the closed `ValidationErrorCategory`. The lenient catch-all alternative described in §1.4 was considered and not taken. From 6e6a7edd1b5316fce5a8b869d4a4a688adc69a01 Mon Sep 17 00:00:00 2001 From: Louis Choquel Date: Wed, 30 Sep 2026 00:27:04 +0200 Subject: [PATCH 2/2] Cite the shared prepare_inputs design by its ledger id The prepare_inputs module docstring and the input-preparation status note still named pipelex-sdk-js's wip/ design, which moved to the workspace. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DdKRGdm7qxgvXuspUpJLj9 --- docs/input-preparation.md | 2 +- pipelex_sdk/prepare_inputs.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/input-preparation.md b/docs/input-preparation.md index 43929c4..a0a69c3 100644 --- a/docs/input-preparation.md +++ b/docs/input-preparation.md @@ -1,6 +1,6 @@ # Input preparation (`upload_file` / `prepare_inputs`) -> **Status: implemented** (`pipelex_sdk/upload.py`, `pipelex_sdk/prepare_inputs.py`). `upload_file` and `prepare_inputs` are the Python counterpart of `@pipelex/sdk`'s `uploadFile` / `prepareInputs`, built on the raw `upload()` wire call. The design of record for the current shape is `pipelex-sdk-js/wip/prepare-inputs-selectors/design.md` in the sibling repo; the two SDKs are kept semantically identical. +> **Status: implemented** (`pipelex_sdk/upload.py`, `pipelex_sdk/prepare_inputs.py`). `upload_file` and `prepare_inputs` are the Python counterpart of `@pipelex/sdk`'s `uploadFile` / `prepareInputs`, built on the raw `upload()` wire call. The design of record for the current shape is shared with `@pipelex/sdk` and tracked as L-260829-300c50 in the workspace ledger; the two SDKs are kept semantically identical. > > **Current scope.** `prepare_inputs` names the method three ways — inline `files`, a `method_ref` address, or a stored `method_id` — and reads the target pipe's signature from the standard's input-form descriptor. One piece is deliberately deferred and additive (it does not change this contract): the opt-in ingest of `http(s)` URLs into storage — for now an `http(s)` URL at a file position always passes through unchanged. diff --git a/pipelex_sdk/prepare_inputs.py b/pipelex_sdk/prepare_inputs.py index b189c05..484e0ee 100644 --- a/pipelex_sdk/prepare_inputs.py +++ b/pipelex_sdk/prepare_inputs.py @@ -14,8 +14,8 @@ field merely named `url` was read from disk and uploaded. The descriptor states the resolved kind at every depth and includes optional fields, so both are gone. -See `docs/input-preparation.md`, and the design of record in -`pipelex-sdk-js/wip/prepare-inputs-selectors/design.md`. +See `docs/input-preparation.md`. The design of record is shared with `@pipelex/sdk` and +tracked as L-260829-300c50 in the workspace ledger. """ from __future__ import annotations