UN-4008 [FEAT] Add the unstract CLI to run extractions and API deployments from the terminal - #2
UN-4008 [FEAT] Add the unstract CLI to run extractions and API deployments from the terminal#2chandrasekharan-zipstack wants to merge 87 commits into
Conversation
Wheel skeleton for the `unstract` console script: Click app with the
whisper / docstudio / config groups, and the three cross-cutting layers
every command will sit on.
- config: named profiles resolved flag > env > profile > default, with
`env:` indirection so the file records where a secret lives rather than
the secret, 0600 writes, deployment aliases, and `config doctor`
reporting where each setting resolved from without echoing a value.
- output: one JSON envelope {ok, data, error, meta} on stdout for success
and failure alike, so parsing never depends on TTY detection; table and
raw are opt-in renderings, diagnostics go to stderr.
- errors: the exit-code table as a stable API, retry policy that never
retries a 4xx, redaction, and undeclared statuses reported verbatim
rather than guessed.
- poll: transport-agnostic --wait loop reading terminal state from the
response body rather than the HTTP status, never sleeping past the
deadline, echoing the job handle on timeout so work resumes instead of
being resubmitted, and persisting a one-shot result before the read is
acknowledged.
No transport yet: the clients own HTTP. Tests are offline and need no
credentials.
Flags for an operation come from the spec the published client is generated from, intersected with what that client's signature actually accepts: a spec parameter the frozen client cannot name would raise TypeError at the call rather than reach the API, so it is not offered. Two rules keep the derivation honest. Every option defaults to None, meaning absent, so an unpassed flag is not sent and the client or server default applies rather than a value pinned here. And only None is treated as absent: 0, false and "" are choices a caller made and travel to the request. Help text has three sources in order: the overlay, the spec, and the client method's own docstring, which is the only one that describes the parameters today. The overlay carries what a generated spec cannot express -- allowed values, short flags, wording -- in TOML read with the stdlib.
Thirteen commands: whisper extract/status/retrieve/detail/highlights/usage and its four webhook commands, plus deployment run and status. Each one holds only what a spec cannot say -- which parameter is the argument, which the CLI owns, and how a result is polled for. The CLI runs the poll loop for both products rather than using the loop one client ships, so --wait, --interval, --timeout and the handle-returned-on- timeout behaviour are the same everywhere. Deployment runs are queued (timeout=0) so a request does not hold a connection open for the length of the job. Line-highlight scaling is arithmetic on a reply rather than a request, so it is folded into the command that fetches the metadata. Failures converge on one envelope: LLMWhisperer raises with a status code, the deployment client returns one, and both become a CLIError with an exit code and a hint. A result that can be read only once is written to disk before it is printed.
--discover answers what --help answers, as JSON, in three tiers: groups names the products, summary adds their commands, full adds every flag with its type, choices and default plus the exit-code table -- enough to construct a call without a second round trip. A caller starts cheap and drills down. Every tier is read back from Click itself, so a described command cannot drift from the one the parser accepts, and discovery reads no configuration: it is how a caller learns what exists, so it has to work before anything is set up. config doctor --probe adds the second diagnostic question -- does the resolved key work -- to the one it already answered offline, where it resolves from. LLMWhisperer is checked against its usage endpoint. A deployment has no side-effect-free endpoint to call, so its entry reports that the settings resolve and says plainly that nothing was verified.
The vendored specs and the pinned clients move independently, so a refreshed spec can declare a parameter the published client has no argument for. Such a parameter is dropped rather than offered and rejected at the call, and dropping it silently is the failure this pins: the gap is written down per operation, so widening it is a decision rather than an accident.
Two failures a live call found and no offline test could. The metadata arrives as a named object carrying the coordinate list under `raw`, while the client's geometry takes the bare list, so no line was ever scaled. And a line the service has no geometry for is reported as all zeros, whose page height is a divisor in that scaling: it raised ZeroDivisionError out of the client, which the entry point does not catch, so the command printed a traceback with an empty stdout. Such a line now gets no box.
Three follow-ups to the command surface. Both client pins move forward, and the six extraction parameters and three status parameters they gained appear as flags with no line written here -- which is what deriving from the specs was for. The contract test's unreachable set shrinks to what the clients own rather than lack: the URL-in-body flag and the execution id read from the endpoint URL. --base-url, --api-key and (for deployments) --org-id sit on the product group and fill the flag tier of flag > env > profile > default, which the loader already supported but nothing populated. A key given on the command line warns: it lands in shell history and in the process list. The 406 hint is scoped to deployments. A whisper result read twice comes back as a 400 whose body says so, and translating on that prose would break the moment the wording changes -- the service's own message already says what happened, and it is passed through verbatim.
`deployment status` derived --include-metadata, --include-metrics and --include-extracted-text from the spec, collected them into **params, and never passed them to the client. The command succeeded and the payload parsed, so a dropped flag was indistinguishable from a working one. The poll loop behind `deployment run --wait` had the same hole, which made a waited run return less than the identical flags returned without --wait. Both now forward what was asked for, and the parameters the status endpoint does not accept are filtered out rather than sent. Tests cover each flag in both polarities, since a flag silently dropped is exactly what the offline suite missed. Alongside: - `config doctor` no longer reports an `org_id` setting for LLMWhisperer, which has none. It always read as unresolved and there was no way to resolve it. - The deployment probe reports `ok: null`, not `ok: true`. Nothing is called, so there is no verdict; `true` beside `checked: false` reads as a live check that passed. `resolved` carries what is actually known. - The 406 hint pointed at --save, which does not exist on the command that emits the hint. It now names the command that has it. - A 400 carries a hint. The service can answer 400 with an empty error body, in which case the message was a synthesised fallback and there was nothing else to go on. Adds RUNBOOK.md: install, moving the client pins, the live-gate checklist, and the release steps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Waiting returns the result and nothing else: the extracted text, or the deployment's structured output. Neither names the job, so a caller who waited had no handle to correlate against the service, quote in a bug report, or use for a follow-up call. Without --wait the handle is the entire payload, so the identity appeared and disappeared depending on a flag. Both waited paths now carry it in `meta` -- the whisper hash and the execution id -- leaving `data` exactly as it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
`--save` exists to protect a read the service serves exactly once, and it
was the flag that lost the data: the write ran after the acknowledging
read, raised `OSError` through an entry point that does not catch it, and
left an empty stdout with the extraction gone. The target is now proven
writable before anything destructive runs, the write goes through a
temporary file so a full disk cannot truncate the previous copy, and a
write that fails anyway raises with the payload attached under its own
exit code -- by that point the envelope carries the only copy left.
Also on the one-shot path: a waited extract read the result with a bare
`.get("extraction")` where the sibling command falls back to the whole
payload, so a response shaped any other way printed `ok: true, data: null`
for a document that had been processed and billed. Both now read it the
same way, and a genuinely empty result is a failure rather than a silent
success.
Redaction was an opt-in keyword argument that only the success path
passed, so every error envelope and every stderr summary went out with
the key in it -- four times on stdout in the reproduced case. Credentials
are now registered where they resolve and scrubbed by every emitter, and
`CLIError.details` is redacted structurally rather than at each call site.
Three more places where a failure was reported as a success: the
standalone status commands ignored a finished-and-failed execution inside
an HTTP 200, the poll loop treated an unreadable body as progress and then
blamed the timeout on a job it never confirmed was running, and any status
outside 4xx/5xx mapped to exit 0 while printing `ok: false`.
Verified by mutation -- moving the save after the print, dropping the
registry, dropping the details redaction and dropping the status check
each fail the suite now, and none of them did before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
A CLI whose output shape depends on whether a terminal is attached is a CLI whose scripts break when they move from a shell to CI. This drops the isatty question entirely: the default is a table, in a terminal and in a pipe alike, and anything that parses the output asks for `-o json`. An explicit `-o` is the last word. The environment picks the default and nothing more, so the same `-o json` invocation renders the same bytes wherever it runs -- which is the property a caller is actually relying on. Coding agents are the exception worth making: they set a marker in the environment, and there the default becomes json rather than making every call carry a flag. `--agent yes|no` settles it either way. Every envelope now carries `meta.contract_version`, and `--discover full` publishes what a consumer has to do to hold up its end: ignore unknown fields, refuse a version above the one it was written against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Nothing bounded a stalled connection: the deployment client is untimed and its api_timeout is an execution mode the backend reads, not a socket timeout. --transport-timeout sets one. Unset by default, so a run that would have hung still hangs rather than starting to fail in a way no existing script expects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Ctrl-C came back as exit 1 with nothing on stdout, which reads to a supervisor as a failed command worth retrying -- the one thing that must not happen to a run the user deliberately stopped. It now exits 130, the value every shell already reads that way, and prints the same envelope as any other failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Overlay, spec and client docstring can each describe a flag. No spec parameter carries a description today, so the order between them is unexercised until one does, which is exactly when an inversion would ship unnoticed.
The vendored LLMWhisperer spec was several revisions behind and now declares enums the CLI was hand-listing. The two had already diverged: --mode rejected three modes the service accepts and --output-mode two, and nothing would have reported it. Read the enum off the spec, keep the overlay for narrowing one on purpose, and drop the descriptions' own value lists for the same reason their default sentences are dropped. `highlights` gains a `mode` query parameter that the published client has no argument for, so it joins the parameters the CLI cannot reach.
A sentence-shaped match ends at the first period, so "Defaults to 0.3." was left in the help beside the default rendered from the signature. Strip each restated sentence with its own end-anchored pass instead.
The pinned clients predated the fix that stops an omitted optional parameter being sent as the string "None", so a CLI built on them sent it. The derived surface is byte-identical across the move; neither signature changed.
Each of these restated the line below it, or described a prior state that is no longer there to check against. Keep the reason, drop the narration.
Copies one organization's resources into another by calling the client's orchestrator directly. Two endpoints with a key each, which no single profile describes, so both are flags and both keys come from the environment. Also moves the client pin forward to pick up the status path-prefix fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The vendored copy was several iterations behind the one the pinned client is generated from, so the CLI's help, its parameter set and what --discover publishes all described an older service contract. The flag snapshot is the check that makes a resync safe: every other contract assertion reads the spec on both sides of its comparison, so a spec that loses a parameter loses the flag and the expectation with it.
…tatus A transport error was translated into a CLIError outside the poll loop, where the handle no longer exists, so the caller was left to resubmit a document the service had already processed and billed. Translating at the call keeps the loop's own context; the loop attaches the handle itself for anything the caller did not translate. `whisper status` reported a failed extraction as a success, its sibling in the other product having already been fixed: both read the body, not the status code.
Four failures the CLI reported as successes or as something vaguer than it knew: - a server-reported error inside a 2xx got the catch-all exit code, which is the least informative one for the most interesting failure this API has; - `config doctor` printed its own findings and exited 0, so a setup script branching on it read a broken configuration as a working one; - a deployment alias pointing at an unset environment variable fell back to the profile's organisation and key, running against a tenant nobody named; - a webhook's auth token was echoed verbatim. The restated-default stripper was also greedy to the end of the string, so a description whose value list came first lost every sentence after it.
The command that writes into a live organisation had none of its own behaviour pinned. Its table output -- the one a person gets, and the only output path that did not go through the emitter -- scrubbed by hand and was run by no test, while the test that claimed a platform key never reaches stdout passed with the registration deleted. Rendered output now goes out through the same path as every envelope, and a key planted in a report is asserted not to survive it. Also: --on-name-conflict decides what is written into the target and is now asserted to arrive; skipped documents are counted at the top of the payload, because skipping is not fatal and a caller reading the exit code alone would never learn a document did not move; `config doctor` resolves each deployment alias the way a run does, instead of listing names its docstring implies it checked; a failed retrieve is pinned to carry the handle; the restated-default stripper ends at its own sentence rather than at the end of the text; and the groups tier lists leaf commands apart from groups, which a consumer walks differently.
The status endpoint's own query parameters are forwarded now, and a deployment URL that carries no derivable prefix is polled where the service said rather than at a rebuilt path.
The notes carry the console-script collision, the behaviours a script would otherwise discover by being surprised, and the service version a custom page separator needs. The pin moves to a documentation-only commit.
The envelope shape is documented in the README and published by --discover; greeting every --help with it buries the two things a reader is there for.
A .unstract.toml found by upward search comes from whatever checkout the user happens to be standing in. It may still select a profile, set org_id and define deployment aliases; api_key and base_url are withheld, with a warning, and reported as withheld by config doctor. Named explicitly with --config or $UNSTRACT_CONFIG, the same file is honoured in full. Also point a first-time user at where keys are minted, from config init, from doctor and from the README, and ship an on-prem profile shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The discovered config path is written to as well as read from, so a symlinked .unstract.toml let a repository redirect config set and config init --force onto any file it named. The upward search now skips a symlinked candidate, and the write opens with O_NOFOLLOW so a symlink at the target is a clear error rather than a truncation. Also: the config group reports the file's warnings instead of dropping them, doctor answers for a withheld deployment-alias key the way it does for a product one, trust is derived from the path rather than from how the loader was called, and the README says plainly that routing stays repo-controllable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Without --config or $UNSTRACT_CONFIG, `config init` took whatever path resolution returned -- including a project .unstract.toml found by walking up from the working directory. Two things went wrong there: the file the caller never named got created or overwritten, and the starter profiles it writes are credential indirections a discovered file is not trusted to supply, so the next command refused the config init had just produced. Discovery stays a way to read a config, not a way to choose where a new one goes. A named path is still the target wherever it points. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
ritwik-g
left a comment
There was a problem hiding this comment.
Standardized PR Review — INITIAL
Run under the team's standardized 19-lens rubric. Base 13cb3f77 -> head f46b650c, 46 files, +14,071/-0; scope reconciled against GitHub's own file count. Reported at a must-flag floor by request — Lows and style are suppressed throughout, so absence of a nit here is not evidence of one.
Verdict: BLOCK
Critical: 0 · High: 9 · Medium: 13 · Lenses run: 19/19
The engineering is careful and the reasoning is written down — redaction is applied before rendering, the poll deadline uses monotonic with an injected clock, config writes are mkstemp+fsync+os.replace, and the trust boundary on a discovered .unstract.toml survived adversarial probing. The architecture is not what blocks this. Nine High-severity defects do, all at seams between components, most proved by reproduction rather than argued. Each has a stated local fix.
Findings are threaded inline at their file:line. Highs are #1-#9.
Lens checklist — 19/19
| Lens | Result | |
|---|---|---|
| 1 | Spec & intent | See unanchored — repo states no title convention, so nothing to judge the title against |
| 2 | Architectural fit | Clean |
| 3 | Correctness & edge cases | See #1, #4, #9, #10, #11, #12, #18 |
| 4 | Security | Clean — vendored security deep-dive completed (not degraded); config trust boundary adversarially probed and holds |
| 5 | Data integrity | See #2, #17 |
| 6 | Concurrency | N/A — single-threaded; the one module global (secrets registry) is documented and test-reset |
| 7 | API & contract compatibility | See #3, #6, #13, #15, #21, #22 |
| 8 | Reliability & resilience | See #19 |
| 9 | Performance & cost | Clean — cold import 0.18-0.31s, specs @cached |
| 10 | Observability | See #16 |
| 11 | Operational safety | See #5, #20 |
| 12 | LLM/agent | N/A — no prompts, model config, or agent loops |
| 13 | Testing | See #7, #14, and the coverage entry below |
| 14 | Dependencies & build | Clean — clients pinned exactly, lockfile resolves 159 hashes from PyPI only |
| 15 | Code quality | Clean — pinned gate passes at ruff 0.16.2, the lockfile's version |
| 16 | Doc & comment accuracy | See #8, #13 |
| 17 | Cross-document rule consistency | Clean — branch current with origin/main, no merge-result sweep needed |
| 18 | On-prem deployment experience | N/A — no chart, on-prem values, schema, build path, or customer docs |
| 19 | Scaling & workload lifecycle | N/A — no pooled worker, autoscaling, or lifecycle config |
Lenses 4, 5, 6, 8, 11, 12, 14, 17, 18, 19, lens 7's consumer search and lens 15's pinned gate were assessed by the orchestrator rather than a dispatched agent. A data-flow boundary sweep traced 11 output sinks: clean.
Coverage gaps — folded into one entry, all mutation- or coverage-proven
Not padding, and not a request for more tests where coverage exists — each is a failure path with none:
translated()'sTimeoutbranch is unexercised, and only clause order keepsConnectTimeout(whose MRO isConnectTimeout -> ConnectionError -> Timeout) out of theConnectionErrorarm. Reordering silently changes a published exit code.core/clients.py:213-220clone'sPlatformAPIError/CloneError/report.abortedtranslation is entirely untested —PlatformAPIErroris the shape every non-2xx Platform API answer arrives in.commands/clone_cmd.py:182-199, 221-222, 263whisper webhook updateanddeleteare untested, including theremember_secretcall that stops a fresh--auth-tokenechoing back. The identically-shapedwebhook gethas that test.commands/whisper_cmd.py:381-386, 411-415__main__'sBrokenPipeErrorandOSErrorarms are untested.unstract ... | head -1is ordinary usage.__main__.py:73-75, 79-84
Suite coverage is 94% (1698 statements, 104 missed). Defensive branches are deliberately not filed.
Unanchored findings
- The PR description asserts a mechanism that does not exist. It states "Config writes use
O_NOFOLLOW, so a planted symlink cannot redirectconfig setonto some other file."O_NOFOLLOWappears nowhere in the repository. The security property does hold —os.replaceacts on the final link, not its target — so this is a wrong claim rather than a vulnerability. Flagging it because it is the sentence a reviewer would rely on to skip checking the write path. redact_headersis dead code, referenced only by tests.- The spec pin is self-certifying.
tests/test_specs.pycompares each vendored spec against asha256inprovenance.json, and both move in the same edit — so it catches a corrupted copy, not a hand-edited one, and nothing verifies therepo/commit/pathit names. Thebump-client-pinsskill states this limit correctly and explicitly;src/unstract_cli/specs/README.md:13-14reads as stronger protection than exists. - Nothing ties the client pin to the spec commit at release time. Bumping a client without re-syncing its spec fails no test; the CLI just silently stops offering a parameter the new client gained. That direction only loses a feature, so it is the benign half.
Evidence and its limits
Every High was reproduced or verified against the pinned clients in isolated virtualenvs outside the worktree; the review worktree was never written to. One exception, stated in the finding itself: #9 depends on service behaviour nobody could settle without live credentials.
Verdict posted as a comment, not as REQUEST_CHANGES — the merge gate is the maintainers' call, not the review's.
The clients answer with an empty string for a field that has no value yet rather than omitting it, so raw output stopped at the first declared field every time and printed a blank line for a queued run instead of its handle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Where `details` is the only surviving copy of a result the service will not serve again, redacting it by field name destroys the part of the answer the caller is being handed it to recover. The literal scrub of every resolved credential still applies on the way out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Reading `-o` out of argv by hand only recognises the spellings it was written for, so a clustered `-ojson` succeeded as JSON and failed as a table. The root callback now fills in a context the entry point holds, leaving the argv scan for failures that happen before any of it has been parsed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Resolving a credential to scrub for goes through the context's config, which raises a CLI error rather than the config error the handler was written for. A command that takes its endpoints as flags would then exit on a config file it never needed, discarding a report of work already done. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The handle a run polls on is a status URL, and the status command takes an execution id, so a timed-out run told the caller to resume with something no command accepts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The alias branch, the bare-name fallback and the URL the client reads its organisation back out of were reached only through tests that stub the builder out entirely, so nothing exercised the route itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
A tag, a branch and a release can all be deleted; a version on PyPI cannot. Publishing first meant any later failure left a released version that no tag in the repository names, which is the one outcome that cannot be cleaned up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
An alias that names its own org or key keeps them: the flags fill in only what it leaves to the profile, while --base-url is not per-alias and always applies. Pinned by a test so the documented precedence cannot drift silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Zero seconds between polls is a busy loop against a metered service, and doubling a zero interval never grows it, so the backoff after a rate limit answered at the rate that earned it. The flag now refuses it and the loop floors it for callers that do not come through a flag. Discovery publishes the bounds as their own keys: Click names a bounded number "float range", which is not a type a caller can map onto anything. The sleep seam is resolved on the call rather than captured at import, so replacing it in a test reaches the loop -- which it previously did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The result is written to a temporary sibling and moved over the target, so the directory is what must be writable. Opening the target itself passed for a writable file in a read-only directory and failed only after the one-shot read the flag exists to protect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Everything the service raises on purpose is already a CLI error by the time it reaches the poll loop, so what the bare handler catches is this side's own bug. Labelling it a retryable server error repeated it until the retry budget ran out and then blamed the service. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The CLI leaves these flags without a Click default so that nothing is resent, which left the value the client or the service applies readable only as a sentence inside the help text. Discovery now carries it as `server_default`. The spec states some of those defaults in prose of its own, so a flag could carry two statements of one default -- and one of them was already wrong. Stripped, leaving the rendered value as the single statement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The check looked for `$GITHUB_TOKEN` spelled out in a run block and so matched nothing: `gh` takes its credential from the environment without naming it. It now matches the steps that shell out to `gh`, and a guard fails if that stops matching anything rather than letting the check pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Printing help on stdout and exiting 0 tells a parser the run succeeded and then hands it a page of prose in place of the envelope. A group invoked with no command already answers with a usage error; the root now does the same, and a person still gets the help page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The config, overlay and credential registries are imported by the output layer and so cannot import it back; each wrote straight to stderr, which left three notes that --quiet did not reach. They now go through a sink the run binds. Notes raised while the command tree is built are held until there is a run to ask, and anything still held when the entry point returns is written out rather than dropped for having been early. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
`save_config` rebuilt the document from `default_profile` and `profiles` alone, so writing a profile deleted every other top-level table in the file. The parsed mapping is kept on `ConfigFile` and the write starts from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
A skipped, oversize or unsupported file does not fail the run, so the exit code says nothing about it and only the table renders the counts. The summary now goes to stderr in every format, under --quiet like any other diagnostic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
`config set` wrote any key under any product, so a typo was stored and silently never read; `doctor` now names a key already sitting in a product block that nothing resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
An entry for an unknown product, operation or parameter applied nothing and said nothing, so a short flag or a narrowed value list could be written and never take effect. The flag snapshot also records what the overlay resolves, so a narrowing that stops applying moves it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Timeout translation and the clause order that decides where a connect timeout lands, the clone command's Platform API, start-up and abort translations, `whisper webhook update`/`delete` with the token they carry, and the entry point's closed-pipe and OSError arms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
No caller builds or prints a header map, so this redacted nothing while reading as if headers were covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The deployment client sets none of its own, so a stalled connection was waited on forever unless the flag was passed. Default to the 120s the LLMWhisperer client applies; `--transport-timeout 0` keeps the old behaviour for a caller who wants it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
`create-github-app-token@v3` takes the App's client id, and the org's existing variables are named for it, so this repo can share them rather than needing an App of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The hash and the file move in the same commit, so the check cannot tell a deliberate edit from a refresh. It catches a copy that was corrupted or half-updated, and a provenance entry left behind by its file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
provenance.json now records the exact pin each spec was copied for, and tests/test_specs.py compares it with the pin in pyproject.toml. A client bumped without its spec re-synced fails locally, with no network, instead of deriving flags the released client cannot carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The service reports a batch as COMPLETED even when a document inside it failed, with the failure carried per file in extraction_result. The CLI read only the execution status, so a caller branching on the exit code was told the batch succeeded with a document's output missing. Both `deployment run` and `deployment status` now walk the per-file results and fail with the failed files named; the full payload is kept verbatim in error.details, since the status read is one-shot and the successful documents survive nowhere else, and --save still writes it before the error is raised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
ritwik-g
left a comment
There was a problem hiding this comment.
Standardized PR Review — FOLLOWUP (rounds 2 and 3, combined)
Scope: f46b650c..1ac518e7 — the 26 commits since my first review. Base unmoved (13cb3f77), branch current. Scope change: NO — no new files, dependencies, persisted fields or network calls.
Verdict: REQUEST CHANGES
Critical: 0 · High: 3 · Medium: 5 · Prior findings resolved: 21 of 22
Down from BLOCK. Every High below is a regression introduced by a fix, or a guard on a fix that cannot fail — not a surviving defect from the original PR. Each has a one-line remedy.
Prior findings — reconciliation
RESOLVED (21): #1-#8, #10-#19, #21, #22, the coverage entry, and all four unanchored items.
Three were re-proved by mutation rather than taken on trust — #7 (clients.py, both original mutations now fail), #14 (test_workflows.py, and its anti-vacuity guard is itself load-bearing), and #9.
#9 deserves a specific note. It was the one High left open pending a live run, and 1ac518e settles it: "The service reports a batch as COMPLETED even when a document inside it failed." So it was real, not void. The fix walks per-file results on both run and status, sets verbatim_details=True so the successful documents survive the error path, names the failed files in extra, and updates the exit-code table. I verified the --save-before-raise ordering on both paths myself (poll.py:266-272 inside wait_for_completion; docstudio_cmd.py:285 before :287). Good fix.
NOT RESOLVED — deliberately, with reasoning I accept (1): #20, tracked as issue #7 — nothing on PyPI to pin to until the release workflow runs once.
Two claims I investigated and rejected
Recorded because passing them on would have wasted your time:
- "
preflightno longer rejects a directory--savetarget." False positive. All three--saveoptions declareclick.Path(dir_okay=False); Click raisesBadParameterbeforepreflightruns. Reproduced by callingpreflight()directly, which bypasses type conversion. - "
PUSH_TO_MAIN_APP_CLIENT_IDmay not be set." Already satisfied — it is available to this repo org-wide, and in use in three other Zipstack repos.
Release-path status, since it came up in Slack
All three prerequisites are now met. The org variable and secret were already shared with this repo, the App was already installed org-wide (repository_selection: "all"), and the main ruleset now carries zipstack-push-to-main (actor_id 1199465, Always) in its bypass list — verified, with all four rules intact. Only step 3 ever actually needed doing.
Lens checklist — 19/19
Unchanged from round 1 except: 3 → #1, #2, #8; 7 → #1; 11 → #4; 13 → #3, #7, #8; 16 → #5, #6. 4 Security — Clean, with the open question below. 15 — Clean; pinned gate passes at ruff 0.16.2 (the lockfile's version) on 1ac518e7. 17 — Clean, branch current. 6, 12, 18, 19 — N/A.
Coverage caveat, stated rather than hidden: lens 3 over commit 1ac518e was assessed by me directly, not by a dispatched specialist — that agent terminated on a rate limit and returned a fragment. I read _failed_files / _raise_for_failed_files and both call sites in full and found the logic sound, but this line had one reader rather than two.
Open question for the author
verbatim_details=True means a credential the run never resolved, appearing under a secret-looking key in a server-authored body, now reaches stdout where it was redacted before. Nobody could trace a concrete route — FileResult.metadata is untyped in the vendored spec, which is the gap. Does include_metadata on a deployment status response ever carry adapter or connector credentials? If it can, poll.py:172/:205 and docstudio_cmd.py:207 need redaction on everything except the result-bearing key rather than being skipped wholesale.
Unanchored observation
_failed_files treats any per-file status that is not casefolded "success" as a failure, while the execution-level poll in the same file accepts two success spellings (terminal_success=("COMPLETED", "SUCCESS")). The spec types FileResult.status as a bare string with no enum. The new tests do cover case variation ("Success", "SUCCESS") and the all-succeeded direction, and you confirmed the shape against the live service — so this is a robustness note, not a defect. Worth a comment naming the spelling you observed, since the spec does not pin it.
Posted as a comment, not REQUEST_CHANGES — the merge gate is the maintainers' call.
| # field that has no value yet as `""` rather than leaving it out, | ||
| # so stopping at the first present key would print a blank line | ||
| # where a later field carries the handle the caller can act on. | ||
| if (value := source.get(name)) not in (None, ""): |
There was a problem hiding this comment.
[High] [Lens 3/7] — the fix for finding #1 was reasoned from one product and applied to both: -o raw now prints a job handle where text belongs, and loses a one-shot retrieve
Found independently by four of six reviewers, each reproducing it by execution.
The guard moved from is not None to not in (None, ""). The premise is real but docstudio-specific: unstract-client==1.6.0 spells a not-yet-available extraction_result as "" and flips pending: True for SUCCESS and not extraction_result, so docstudio never surfaces a terminal empty result. llmwhisperer-client==2.9.0 does neither — whisper_retrieve returns the body verbatim, so result_text == "" on the whisper path means exactly one thing: a document that genuinely extracted to no text (blank page, image-only PDF under a text mode, an out-of-range page selection).
Two consequences, both on the primary -o raw path:
whisper retrieve <hash> -o raw—RAW_TEXT = ("result_text",)is a single-field tuple, soraw_valuefinds nothing and raises.render()runs beforeemit_textwrites, so stdout carries an error envelope and never the extraction. That read is the acknowledging one-shot; without--savethe result is gone. Exit 1, where it previously printed an empty line and exited 0.whisper extract doc.pdf -o raw—EXTRACT_RAW = ("result_text", "whisper_hash"), so it falls through and prints the hash.unstract whisper extract blank.pdf -o raw > out.txtwrites a hash into the text file, exit 0, nothing on stderr.
The comment deleted in this same commit named the outcome exactly: "skipping it would print the next field -- a handle where the caller expects text." The replacement tests are docstudio-only; the ACK/PENDING_STATUS fixtures were edited from None to "", and no whisper fixture covers an empty result_text.
Fix: make "empty counts as absent" a property of the field list rather than of raw_value — or collect the first non-empty hit and fall back to a present-but-empty one before raising. Either keeps the docstudio pending case and restores an empty extraction as a real answer on both whisper paths.
| found := handle.get("execution_id") | ||
| ): | ||
| exc.hint = ( | ||
| f"Resume with `unstract docstudio deployment status {target} " |
There was a problem hiding this comment.
[High] [Lens 3] — the fix for finding #6 turned prose into a paste-ready command, and the command drops --save
The old hint was prose; this one is something a user copies verbatim:
Resume with `unstract docstudio deployment status <target> <id>` rather than resubmitting the document.
No --save. Trace the path: deployment run <target> doc.pdf --save out.json preflights the save, polls, times out. Nothing has been written — deliver (poll.py:266-272) only runs on terminal success. The caller pastes the hint, and by this CLI's own model the status call is the acknowledging read — errors.py:401: "a deployment serves its result exactly once; re-running the status call cannot recover it."
So a caller who explicitly asked for --save spends the one-shot read to a terminal with nothing on disk. deployment status has its own --save (docstudio_cmd.py:204) that would have kept it.
Fix: append the flag when the run carried one — f"... status {target} {found} --save {save}"; save is already in scope at this point. Otherwise mention that --save keeps the result.
| assert error["failed_files"] == ["bad.pdf"] | ||
| assert error["execution_id"] == "e1" | ||
| assert "bad.pdf" in error["message"] | ||
| # One-shot read: the successful documents survive only here, unredacted. |
There was a problem hiding this comment.
[High] [Lens 13] — the only assertion on the rescued payload cannot fail when the payload is redacted
verbatim_details=True (docstudio_cmd.py:207) is the line that keeps the one-shot result whole. Without it CLIError.to_dict runs redact_value(self.details), which collapses any key whose name segments hit a credential word.
The fixture's result bodies are {"total": 1}, None and {"total": 3} — and names_a_secret("total") is False. So the redacted and verbatim payloads are byte-identical here, and assert error["details"] == PARTIAL_FAILURE passes either way.
Mutation-proven: deleting verbatim_details=True at docstudio_cmd.py:205-207 leaves 398 passed, 1 skipped — the full baseline, no test named. The comment on line 1481 asserts exactly the property the assertion does not pin.
The consequence of a future edit dropping the opt-out is silent: a caller's extracted field named policy_key, access_key, token or plain key comes back ***REDACTED***, and because the status read is one-shot that value survives nowhere else. Confirmed: redact_value({'result': {'policy_key': 'X', 'total': 1}}) → {'result': {'policy_key': '***REDACTED***', 'total': 1}}.
Fix — one word in the fixture: give one succeeded entry a result key the redactor matches, e.g. "result": {"policy_key": "PK-1", "total": 1}. The existing assertion then fails the moment the opt-out is lost.
The code is correct today; this is the guard on it that cannot fail.
|
|
||
| echo "Created release v$NEW_VERSION" | ||
|
|
||
| - name: Publish to PyPI |
There was a problem hiding this comment.
[Medium] [Lens 11] — the reorder fixes finding #5 but trades one unrecoverable state for another, and the comment overstates what survives
The order is now push-main → tag → release → publish. That does fix the original hazard. But uv publish is the step most likely to fail transiently (PyPI outage, OIDC hiccup, network), and when it does you are left with a public GitHub release for a version that is not installable: main carries the bump, tag vX exists, the release is live, PyPI has nothing.
The retry is blocked. version_bump: none recomputes X and exits at the existing-tag guard (:105-108 — fetch-depth: 0 at :51 means the tag is visible), while patch publishes X+1 and leaves X permanently absent behind a live release.
The comment at :131-134 says "a tag, a branch and a release can all be deleted" — omitting the fourth artifact, the bump commit already pushed to the default branch, which needs a revert push rather than a delete.
Fix: create the release as a draft and un-draft it after uv publish succeeds; or let a retry skip the tag/push steps when vX already exists rather than exiting 1. If the order is intentional as-is, at minimum correct the comment to name the commit on main and record the manual cleanup sequence.
| def warn(message: str) -> None: | ||
| """A note from a module that cannot reach the output layer. | ||
|
|
||
| The config, overlay and credential registries are all imported by it, so |
There was a problem hiding this comment.
[Medium] [Lens 16] — warn()'s docstring states an import relationship that does not exist
The docstring justifies the sink indirection with "The config, overlay and credential registries are all imported by it [the output layer], so they cannot import it back."
output.py imports only errors — output.py:32-38 is its entire first-party import list. Neither config.py nor overlay.py is imported by the output layer, and neither would create a cycle importing it (output → errors → stdlib). Both import from errors, not into it.
A maintainer reading this believes an architectural constraint binds three modules when it binds one, and will preserve or extend the held-note machinery for modules that could call diagnostic directly.
Fix — prefer deletion over a rewrite: drop the second sentence, keeping "this is the seam that keeps their notes subject to the same --quiet as every other diagnostic." The rule is already stated correctly and scoped correctly at app.py:176-177 — "Modules the output layer imports cannot import it back" — which is true of errors.py, the module that actually has the constraint.
| | `docstudio.json` | `specs/docstudio-oss.json` in the backend, generated by `manage.py generate_docstudio_spec` | | ||
|
|
||
| `provenance.json` records the client pin each copy was synced for, the commit | ||
| it was taken from and its sha256. `tests/test_specs.py` fails if a vendored |
There was a problem hiding this comment.
[Medium] [Lens 16] — the pin guard is described as catching something it cannot
"tests/test_specs.py fails … if the pin in pyproject.toml has moved without the spec being re-synced."
The test compares provenance.json's client string against the pyproject.toml pin — two hand-edited strings. Bump the pin, edit client to match, leave docstudio.json untouched: all three tests pass. The sha256 test passes because the file is unchanged, the entry-set test passes, and the pin test passes because the two strings now agree.
The guard is a prompt to re-sync, not proof that re-syncing happened. This paragraph's whole purpose is to say what the check does and does not catch, and the following sentence never corrects it because that one is about the hash only.
Fix: delete the over-strong clause and let the sentence stop at the hash. .claude/skills/bump-client-pins/SKILL.md:55-58 already states the same guard accurately — "fails if a vendored file stops matching its recorded sha256, if its client no longer equals the pin in pyproject.toml, or if a spec has no provenance entry" — naming the mechanism without promising it detects a stale copy.
| assert payload["error"]["code"] == "interrupted" | ||
|
|
||
|
|
||
| def test_a_reader_that_went_away_does_not_raise_on_the_way_out(capsys, monkeypatch): |
There was a problem hiding this comment.
[Medium] [Lens 13] — the only BrokenPipeError coverage cannot fail on the behaviour it claims
This is the sole test of the arm at __main__.py:73-79, and it exercises none of it. Under capsys, sys.stdout is a CaptureIO over a BytesIO, so sys.stdout.fileno() raises io.UnsupportedOperation — which subclasses OSError and is therefore swallowed by the enclosing contextlib.suppress(OSError). The os.dup2(…devnull…) redirect never executes.
The trailing print("still writable") then writes to an in-memory buffer that was never broken — the BrokenPipeError came from a monkeypatched load_config, not from stdout. What survives is only the exit-code assertion.
Mutation-proven: replacing both lines of the suppressed block with pass leaves 394 passed, 1 skipped, fully green. grep -rn BrokenPipe tests/ returns only this line, so nothing else covers it. unstract whisper extract … | head -1 is ordinary usage for this CLI, and the redirect is what stops a second BrokenPipeError during interpreter-exit flush.
Fix: use capfd instead of capsys — it gives stdout a real file descriptor, so fileno() succeeds and dup2 runs — then assert the redirect happened, or spy on os.dup2.
| if not isinstance(entry, dict): | ||
| continue | ||
| status = entry.get("status") | ||
| if status is None: |
There was a problem hiding this comment.
[Medium] [Lens 13] — the status is None branch is the one shape of the defect no test exercises
This is a second detection rule, for a per-file entry carrying an error but no status key — a real shape, since the vendored spec lists only file as required on FileResult.
No entry in PARTIAL_FAILURE omits status, so the rule is never reached.
Mutation-proven: replacing the branch body with pass leaves 398 passed, 1 skipped, the full baseline. Contrast the sibling rule one line below, where dropping .casefold() fails 3 named tests — so the coverage is real for one rule and absent for the other.
If this rule is wrong or gets removed, a batch whose failures arrive in that shape goes back to exit 0 with a document's output missing — the exact defect this commit exists to fix — and nothing notices.
Fix: add a fourth entry to PARTIAL_FAILURE with "file": "noStatus.pdf", no status key and a non-empty error, and extend the two failed_files assertions to ["bad.pdf", "noStatus.pdf"]. That covers both rules from the fixture already in place.
What
unstract— one command that takes a document to structured JSON: LLMWhisperer extraction, an API deployment run, and the polling in between.unstract config init | list | get | set | doctorunstract whisper extract | status | retrieve | detail | highlights | usage,whisper webhook create | get | update | deleteunstract docstudio deployment run | statusunstract --discover groups | summary | fullWhy
Both products are reachable today only from Python. This makes them scriptable from a shell and drivable by an agent: one output envelope, one exit-code table, and a JSON description of the whole surface so a caller can construct a command without a second round trip.
How
flag > env > profile > default. Discovery is--config→$UNSTRACT_CONFIG→ a project-local.unstract.tomlfound by searching upward (stopping at$HOME) →~/.unstract/config.toml. Values may indirect throughenv:VAR, so a config file can be committed without a key in it. Files are written 0600. Deployments are named aliases inheriting org and key from their profile..unstract.tomlmay not supplyapi_keyorbase_url. A checkout the user did not write would otherwise hand the CLI a key or point it at another host; a path the user names explicitly is a deliberate choice and is honoured. Withheld values are kept and written back to their own file, never carried into another one. Config writes refuse a symlinked target and land throughmkstemp+os.replace, so a planted symlink cannot redirectconfig setonto some other file. Routing (org_id,api_name, profile selection) stays repo-controllable by design.{ok, data, error, meta}on stdout, in JSON by default whether or not stdout is a TTY, so a script gets the same bytes as a terminal.--output tablewraps rather than truncates.--timeout 30returns at 30s — and a timeout carries the handle out so a caller can resume rather than restart.TypeErrorat the call rather than reach the API, so it is not offered, andtests/test_contract.pyrecords which ones those are so the gap widens on purpose or not at all. An unpassed flag is not sent, so the server default applies rather than one pinned here; onlyNonecounts as absent, so0,falseand""travel.--discoveranswers what--helpanswers, as JSON, read back from Click itself, so a described command cannot drift from the one the parser accepts.fulladds every flag with its type, choices and default plus the exit-code table.Can this PR break any existing features
No. New repository, nothing depends on it yet, and it is not published. It reads the two clients through their public APIs only.
Notes on Testing
230 tests, offline by design: no network and no credentials — the clients are replaced at the factory, so what is asserted is which arguments a command hands the client, and what a caller sees on stdout and in the exit code. CI runs ruff and pytest, then the suite a second time against the newest
clickthe pin allows —uv runresolves from the lockfile, and an install in the wild does not. Live round trips are a manual pre-release step.Related Issues or PRs
Pins
Built on
unstract-client==1.6.0andllmwhisperer-client==2.9.0, both from PyPI. The vendored specs are byte-identical copies of the ones those releases were generated from, recorded with their source commit and sha256 insrc/unstract_cli/specs/provenance.jsonand checked bytests/test_specs.py. Neither client installs a console script, sounstractis this CLI's alone;unstract-cliremains as a second name.Release
.github/workflows/release.yml— a manual dispatch that bumps the version, lints, tests, builds, publishes to PyPI withuv publishthrough a Trusted Publisher, and only then commits the bump, tags it and cuts the GitHub release. The version lives only insrc/unstract_cli/__init__.py, read through hatch, so the bump edits one file, and the committed value names the last stable release.pre_releasepublishes a PEP 440 release candidate: the target version is computed as usual, thenrcNis appended, counting up from the rc tags already published for that target, and__version__is left alone because a candidate is not a release. Promoting is the same dispatch withpre_releaseoff.version_bump: nonetargets the version already in the repo, so the first cut isnone + pre_release→0.1.0rc1, andnonealone promotes it to0.1.0.Owner-side setup, none of which exists yet on this repo:
unstract-cli, OwnerZipstack, Repository nameunstract-cli, Workflow namerelease.yml, Environment name blank (the workflow declares no environment).PUSH_TO_MAIN_APP_ID(Actions variable) andPUSH_TO_MAIN_APP_PRIVATE_KEY(Actions secret) must reach this repo. Neither exists at repo level here; both are presumably org-level, and an org-level secret still has to listunstract-cliamong the repositories it is visible to.Zipstack/unstract-cli— the workflow requests a token scoped to that repository by name.🤖 Generated with Claude Code
https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ