UN-4010 [FEAT] Support every API deployment request parameter via a generated transport - #27

Standardized review — verdict: REQUEST CHANGES

Critical: 0 · High: 7 · Medium: 11 · Low: 8 · Lenses run: 17/17

Reviewed against a fixed 17-lens rubric (unstract:standard-review, plugin v0.30.1) at d67dda4b, diffed from merge-base 79f8d099 — 31 files, matching GitHub's count. sdk_docstudio/** was treated as generated and reviewed only for whether the facade depends on details regeneration could change.

The engineering here is strong and worth saying plainly: the generator is pinned and version-checked, gen_sdk.sh fails on generator warnings, the generated tree is stamped DO-NOT-EDIT and lint-excluded, the sdk-drift job uses git add -N so a newly-created file cannot slip past the diff, deps are upper-bounded with stated reasons, and the 1190-line parity suite is genuinely load-bearing. The _status_url origin-pinning is careful work.

The High findings cluster in exactly two places: the new parameters this PR exists to add, and the console-script removal.

Things that came out clean — stated so the silence is legible

• A dedicated security pass found no findings at confidence ≥ 8. Notable verified negatives: follow_redirects=True (client.py:342) is safe because httpx 0.28.1 pops Authorization on any non-same-origin redirect — stricter than requests, which compares hostname only and preserves the header across an https→http downgrade. Multipart filename injection isn't reachable (ntpath.basename + httpx's HTML5 escaping). verify defaults True on both layers and is never disabled. The five added packages all resolve from PyPI with sha256, no typosquats, and h11 is at 0.16.0 (at-or-above the request-smuggling fix).
• Greptile's open P1 on client.py:333 ("cross-origin polling leaks credentials") does not reproduce at HEAD. _status_url strips scheme and netloc before geturl(). I tested https://attacker.example/steal, //attacker.example/steal, https://user:pw@attacker.example/x and http://169.254.169.254/latest/meta-data/ — every one resolves back to the configured origin. That is the same mechanism you used to refute the sibling P1 on :339, which Greptile conceded. Your thread is yours to close; flagging only that it's answered.
• The two test changes I scrutinised hardest are legitimate. tests/test_cli_top_level.py's deletion is justified — the wiring those 50 lines exercised survives in tests/clone/test_cli.py:49-127 through the same monkeypatch.setattr("unstract.clone.cli.run_clone", …) seam. tests/test_retry.py has identical test and assertion counts on both sides (52 and 91); every hunk is either a patch-target move or a fixture gaining ?execution_id=. Mutation-checking the parity gate (renaming a public method) produced 33 failures, so it is not warn-only.

Unanchored findings

These have no RIGHT-side line in this diff to attach to.

• [High] [Lens 7] pyproject.toml (the deleted [project.scripts] block) — a published console script and a public module are removed with no version bump, no deprecation shim, and a release workflow that defaults to patch. unstract = "unstract.cli:main" is gone and src/unstract/cli.py is deleted, so from unstract.cli import main now raises ImportError too. __version__ is untouched at 1.5.3 and .github/workflows/main.yml:11 defaults version_bump to patch — so if the release is cut on the default, 1.5.4 ships both breaks to everyone pinned unstract-client~=1.5.3, and a CI pipeline running unstract clone … gets command not found on a routine patch upgrade. Minimally: state the required bump (major, or at least minor) somewhere the release dispatcher will see it. Better: keep [project.scripts] unstract pointing at a two-line shim that prints "moved to the unstract-cli package; use python -m unstract.clone clone …" and exits non-zero, removed next major.
• [Medium] [Lens 1] The PR description does not mention the console-script removal. "Can this PR break any existing features" says "Two deliberate behaviour changes, both narrow" and lists only file-handle closing and status-URL rebuilding. This is the most user-visible change in the diff, and anyone generating release notes from the body will miss it.
• [Low] [Lens 1, 16] README.md:76-77 still describes async/sync selection as api_timeout=0 / api_timeout > 0. After this diff the decision is made by the effective per-request timeout (client.py:586), which a caller can now override.
• PR title — this repo states no title convention in writing (no CLAUDE.md, no CONTRIBUTING, no PR template), so there is nothing to judge against.
• The description says "372 tests"; the suite collects 377. Cosmetic.
• The description notes "A live round trip against a deployment is still outstanding." That round trip is what settles the two open questions below.

Open questions

1. custom_data — does the deployment echo it back anywhere in the execute or status response, and under which key? No response schema declares it.
2. timeout — is -1 meant to be treated as async by this client (widening the branch to <= 0), or is the docstring the thing to narrow? See the finding at client.py:586; this is a decision, not a defect I should pick for you.

Assumption

specs/docstudio-oss.json here is byte-identical to the copy in Zipstack/unstract#2237 — I diffed them. The response-shape findings raised on that PR therefore apply to the models generated here, and fixing them upstream will move this PR's generated tree.

Lens checklist (17/17)

1 see findings + Unanchored · 2 see findings · 3 see findings · 4 Clean — dedicated security pass plus the redirect/multipart/supply-chain negatives above · 5 N/A — no migrations or persisted state · 6 N/A — no concurrency primitives · 7 see findings · 8 see findings · 9 Clean · 10 see findings · 11 Clean — CI improved here: action SHAs pinned, sdk-drift gate added · 12 N/A · 13 see findings; the deletion and the retry-suite edit were both adjudicated legitimate above · 14 see findings · 15 see findings · 16 see findings · 17 see the gen_sdk.sh provenance finding

Posted as COMMENT, not REQUEST_CHANGES — the merge decision is yours, not the review's.

FOLLOWUP review — verdict: REQUEST CHANGES (one High remains)

Prior findings: 23 · RESOLVED 18 · PARTIALLY RESOLVED 4 · NOT RESOLVED 1
New findings: 1 High (carried over) · 4 Medium · 3 Low
Scope change: NO — the delta is confined to the files the findings named, plus the spec refresh they asked for.

Verified against 893e6b6 from merge-base 79f8d099. Status here is what the code shows, not what the replies claim — every fix below was exercised, and the ones that mattered were mutation-tested (break the fix, confirm a test goes red, restore).

What genuinely landed

The substantive work is real and I want to be specific about it, because most of it is verified by execution rather than by reading:

• custom_data now emits b'{"a": 1, "b": "x"}' — valid JSON, json.loads round-trips. Fixed in the facade, so gen_sdk.sh can't overwrite it. Mutation → 3 failures.
• timeout widened to <= 0, coherent across the branch, both docstrings, the README retry table and the spec default. Measured: -1→3 attempts, 0→3, 300→1. Mutation → 2 failures.
• Error text now surfaces for every non-2xx through both methods — 400/403/404/413 all report the server's detail, and degenerate bodies (null, bare string, list, {}) report rather than raise. Mutation → 11 failures.
• The AttributeError crash is guarded, and the vendored spec is byte-identical to upstream Zipstack/unstract@eddd4b74 (sha256 e453d4f7…), with the generated tree regenerated to zero drift under the CI gate's own check.
• The README command runs — python -m unstract.clone clone … parses and stops at the documented --source-key env var.
• File handles: IsADirectoryError/PermissionError now raise APIDeploymentsClientException with zero leaked handles.
• LocalProtocolError: 4 attempts → 1, as InvalidHeader.
• The packaging test, the explicit-None regression test, the per-part Content-Type assertion and the lifecycle (close()/__enter__/__exit__ under a double-checked lock) are all present and mutation-verified.

That's 18 of 23 fully closed, several with better fixes than I suggested — the _operations() source-level guard and the monkeypatched-warn diagnostics test are both improvements on what I proposed.

Why this still can't be approved

One High is untouched — the console-script removal still ships as a patch release. Details in the inline comment on pyproject.toml. This is the only round-1 finding with no code change at all, and it's the one that reaches users who never read this PR.

Four new Mediums, of which two reach a running deployment: the submit-works-but-poll-fails gap on short ingress URLs, and the 429/409 statuses dropped from the spec while the client still has bespoke Retry-After handling for 429.

Residuals on the partially-resolved four

• #4 — check_execution_status still lets the error text double as extraction_result (client.py:850); the new test asserts only result["error"], so it can't see it.
• #9 — the parsed=None claim is refuted by the dropped statuses above.
• #18 — "returned under each result item's metadata.custom_data" is now specific and falsifiable, which is better, but nothing in this repo backs it: custom_data appears in the spec exactly once, in the request schema.
• #22 — the accepted-divergence list in the suite docstring reads as closed at three items; at least two more (the <= 0 retry boundary, the constructor exception class) are discoverable only inside individual tests. The claim that the <= 0 change was declared "in the suite's module docstring" isn't supported — timeout doesn't appear in lines 1-18.

Three Lows worth a line each

include_metrics is still sent while metrics was deleted from FileResult — one of the two is wrong. The packaging manifest is blind to top-level modules (I re-created src/unstract/cli.py and the test still passed). And the spec's sha256 is a comment enforced by nothing, where BASELINE_SHA256 gets a real assertion — the exact distinction that comment draws.

CI is green across the board (Greptile, lint, clone-tests, sdk-drift). Once the version question is settled this is close.

Posted as COMMENT. The three sibling PRs in this chain — Zipstack/unstract#2237, unstract-llm-whisperer#722 and llm-whisperer-python-client#35 — came out clean on their followup and I've approved those.