Skip to content

feat: Unstract CLI — unified interface for Document Studio, LLMWhisperer, and API Hub#1

Open
hari-kuriakose wants to merge 21 commits into
mainfrom
pr-graft
Open

feat: Unstract CLI — unified interface for Document Studio, LLMWhisperer, and API Hub#1
hari-kuriakose wants to merge 21 commits into
mainfrom
pr-graft

Conversation

@hari-kuriakose

Copy link
Copy Markdown
Contributor

Establishes the unstract CLI — one interface over Unstract's three products (Document Studio, LLMWhisperer, API Hub), built for agent use: declarative endpoint records, machine-readable --discover, stable exit codes, structured errors, and no interactive prompts.

main is an empty root commit, so this PR's diff is the entire codebase.

Architecture

Every command is generated from a declarative Endpoint record in src/unstract_cli/endpoints/. The command tree, flags, help text, validation, and --discover all derive from those records, so help text cannot drift from behaviour and the bundled Claude Skill has exactly one place to edit.

Highlights

  • Config & resolution — profiles, .unstract.toml, --config; resolution order flag → env → profile → default; config doctor reports where each setting resolves from without echoing secrets.
  • --discover — a ~1k-token group map an agent drills into, rather than pulling all 156 commands into context.
  • --wait — declarative execute → poll → retrieve, deciding terminal state from the response body (not the HTTP code, which the deployment API returns as 422 mid-run).
  • One-shot results--save persists them atomically before exit; a second read exits 9.

Fixes from a full end-to-end run

Driving a document through extract → Prompt Studio → export → deploy → run surfaced real friction; root causes were traced in the backend source, which changed two fixes and ruled one out:

  • Exit codes never reached the shellstandalone_mode=False makes Click return the exit code, and main() discarded it, so every failure exited 0 while its envelope reported the real code. Highest-impact fix; caught only by subprocess tests, since CliRunner never runs main().
  • prompt create --profile-managerfetch_response reads the prompt's own profile and never falls back to the project default (unlike its siblings), failing with a message that names a setting it never reads.
  • prompt-studio --challenge-llm — an empty challenge_llm fails an enum check at deploy time; setting it before export keeps the metadata valid.
  • Param.mirror_as — the key-create routes needed the same id in both path and body; mirroring lets --api-id alone suffice.
  • org_id fallback chains and --wait on index-document.
  • Server-side items that can't be fixed here now have help text describing the real behaviour instead of implying a workaround.

Verification

245 tests pass; ruff and mypy clean; --discover emits valid JSON (156 commands).

🤖 Generated with Claude Code

hari-kuriakose and others added 20 commits July 21, 2026 23:52
Generated command tree from declarative Endpoint records covering
LLMWhisperer, deployments, Platform v1, HITL and API Hub.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes Phase 6: update-unstract-cli Skill with docs cross-referencing,
zero-drift regression tests, GitHub Actions CI, shell completions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… files param

parse_mdx_doc scanned all <ApiEndpoint> props, so responseBody fields were
read as request parameters (358 false positives across platform v1). Now
reads only pathParams/queryParams/requestBody.

This surfaced a real bug: workflow.execute used 'file' where the API
expects 'files'.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Credentials intentionally have no CLI flag (shell history/process listings),
and the block name in the hint now matches what config init writes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unfiltered full output was ~63k tokens, too large for an agent to read
speculatively. Selection (--group/--command) and verbosity (--detail) are
independent axes; default is now names+summaries (~4.5k tokens).

Also: compact JSON when piped, global boilerplate omitted from filtered
views, exit 2 with clean stdout when no command matches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keeps --dump-commands as a hidden deprecated alias that warns on stderr,
since it is referenced in the Skill, README and CI. Internal function
dump_commands() renamed to discover().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
--discover is now the only spelling. A test pins the removal so the old
flag cannot creep back via a copied example.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t.toml

Config selection was previously env-var-only. Adds a --config flag and
upward discovery of .unstract.toml (stopping at $HOME), composing with
the existing profile mechanism.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Table mode capped cells at 60 chars and appended '...', silently hiding
the tail of notes, URLs and ids. Cells now wrap across lines within the
terminal width, so the human format is lossless like json/yaml/raw.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The config commands were described by a hand-maintained parallel data
structure (CONFIG_COMMANDS/HandAuthoredCommand) instead of being
introspected, so the index claimed --product/--key/--value for
'config set', whose parameters are positional. An agent following the
index would build a command line the parser rejects.

Now introspected from the real Click commands, like every generated
command. Each entry gains kind (argument|option) and a usage line. The
duplicate description model is deleted so it cannot drift again, and a
test cross-checks every advertised parameter against the real parser.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unstract is the company and the CLI name. It builds three products:
Document Studio (formerly named Unstract), LLMWhisperer and API Hub.

- Product enum now has exactly three members; a new ApiGroup enum carries
  the five API surfaces, with the product derived from it.
- platform/deployment/hitl nest under 'unstract docstudio ...'.
- Config blocks nest by product: [profiles.X.docstudio.platform].
- --discover exposes product, product_name and api_group per command.

Wire paths and UNSTRACT_* env vars are unchanged: the product was
renamed, not the API.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
config get/set now accept only docstudio.platform (or 'docstudio platform'),
docstudio.deployment, docstudio.hitl, llmwhisperer, apihub. Bare group names
and the 'whisper' alias are rejected with exit 2 and a hint listing the valid
targets, so a setting always says which product it configures.

Also: config current keys its output by the same target names, and the
variadic arguments declare metavars so --help and --discover both show
'TARGET... KEY VALUE' rather than an opaque 'ARGS...'.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Profile blocks now have exactly one accepted layout: the API group nested
under its product. A block written any other way is ignored rather than
half-applied, so a config that looks applied but is not cannot mislead.

Fixes a latent bug found while removing them: ResolvedConfig.get/require
normalised only Product, so an ApiGroup key fell through unconverted --
profile values were silently missed and errors read 'ApiGroup.LLMWHISPERER'.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The repo tracked 50 `__pycache__/*.pyc` files, so every commit that ran
the test suite carried dozens of modified binaries and buried the real
diff. They are build output: regenerated on any run, machine- and
interpreter-specific (both cpython-312 and -313 copies were committed),
and never useful to a reader.

`git rm --cached` drops them from the index while leaving them on disk,
so nothing needs rebuilding locally.

Also adds a .gitignore, which the repo had been missing entirely --
covering caches, build output, virtualenvs, and `.unstract.toml`, which
is a local config file that can hold credentials.
Work in progress that predates the gotcha-fix pass; committing it as its
own change so the fixes that follow are reviewable in isolation.

- `config doctor` reports where each setting resolves from (flag, env
  var, profile literal, or profile `env:` reference), which answers the
  question that costs the most time: "the CLI says the key is missing,
  but I set it -- where is it looking?" Backed by
  `ResolvedConfig.resolution_source`, which reports the winning source
  without echoing the secret.
- `--discover` gains the navigable-groups overview: a ~1k-token map of
  the command tree with per-group counts and the exact `drill` command
  for each subtree, so an agent reads the map and follows one drill
  rather than pulling all 156 commands into context.
- `PollSpec.status_field` accepts a tuple of candidate field names, so a
  poll can match a status endpoint whose body spells the state
  differently from the run response.
- Drops the unused `typer` dependency and documents why each remaining
  one is there. `uv.lock` is committed so installs are reproducible and
  hash-pinned.
The CLI runs Click with `standalone_mode=False` so that Click's own parse
errors reach us and can be rendered through the structured error envelope
(SPEC §5.5). That mode also changes how `ctx.exit(code)` behaves: instead
of raising SystemExit, Click *returns* the code from the invocation.
`main()` ignored the return value and unconditionally returned 0.

So every documented exit code -- 2 usage, 3 auth, 4 not-found, 7 timeout,
9 already-consumed (SPEC §5.4) -- was reported to the shell as success. A
validation error printed `"exit_code": 2` inside its own error envelope
while the process exited 0. `set -e` never tripped, `cmd || handle` never
fired, and an agent branching on the status code saw every failure as a
pass. This defeats the exit-code contract the README advertises as a core
promise for agent use.

Return the invocation's result when it is an int (a command that falls
off the end returns None, which is success).

Tested by subprocess, which is the only way to catch this: Click's
`CliRunner` invokes with `standalone_mode=True` and never runs `main()`,
so the existing suite asserted the right contract against a code path the
real binary does not take -- 213 tests passed throughout.
Fixes for the issues hit while driving a document through the CLI end to
end (extract, Prompt Studio, export tool, deploy API, run). Root causes
were traced in the backend source rather than inferred, which changed the
fix in two cases and ruled one out entirely.

`prompt create --profile-manager`. `fetch_response` resolves a prompt's
LLM profile from the prompt's own `profile_manager` field and never falls
back to the project default -- unlike `index_document` and `single_pass`,
which both call `get_default_llm_profile(tool)`. A prompt created without
one is unrunnable, and fails with "Default LLM profile is not configured",
which names the *project* default -- genuinely set, and never read. The
flag makes prompts runnable from birth; the 500 now carries a hint naming
the real fix instead of sending the reader back to `profile set-default`.

`prompt-studio --challenge-llm` / `--monitor-llm`. A deployed run ended in
`ERROR` with a 422 at the very last step. The exported tool's metadata
carries `challenge_llm: ""`, which fails schema validation: the property
is `adapterType: LLM`, so the schema gains an `enum` of real adapter ids
and "" is not among them. (Not the `required` rule, as it first appears --
a present-but-empty key satisfies `required`; verified by running the
validator.) Export already resolves a valid value, so setting it on the
project before `export-tool` keeps the metadata non-empty.

`Param.mirror_as` for the API-key routes. `api-deployment key create`
needed both `--api-id` (URL) and `--api` (body) -- the same value twice.
The path value is now mirrored into the body under its own name, so
`--api-id` alone works. Same fix for the `pipeline key create` sibling.
Both records carry `doc_conflict` so the docs-diff skill does not restore
the flags; the diff also now recognises a mirrored param as present.

`default_from` fallback chains. `deployment` and `hitl` `org_id` fall back
to `platform.org_id` -- the same organization, in a block that starts
empty. Credentials deliberately do not chain: reusing a key across API
groups would be credential confusion, not convenience.

`--wait` on `index-document`, which returns the same `{task_id, status}`
shape as its siblings and was the only async command without it. Indexing
writes no Output Manager row, so the terminal status is the result and
there is no retrieve step.

Not fixed, deliberately: making `--vector-store`/`--embedding-model`
optional at `--chunk-size 0`. Both columns are NOT NULL server-side and
the serializer derives required=True, so relaxing the CLI would trade a
fast local exit-2 for a slower remote 400 -- moving the friction, not
removing it. `RequiredUnless` is kept as a tested primitive for a case
where the server genuinely accepts the omission. Help text now explains
that the values are stored but never queried.

Remaining items are server-side and cannot be fixed here; their help text
now describes the real behaviour instead of implying a workaround exists:
the registry's missing back-reference to the Prompt Studio tool, the
absent DELETE route, `adapter-choices` (routed to a method that does not
exist anywhere in the backend, so it 500s on every call), and the empty
default-triad response meaning "unset" rather than an error.
Additive reconciliation of SPEC, README and the update-unstract-cli
Skill against the fixes in the previous commit. Nothing here contradicts
what was already written -- the gaps were mechanisms the docs did not yet
mention.

SKILL.md matters most: it is what a future skill run reads to learn which
encodings exist, so a gap causes real errors later. It now covers
`mirror_as`, the `default_from` fallback chain, and `RequiredUnless`
(listed as available but explicitly *unused*, with the reason, so it is
not reached for where the server enforces the field anyway). The
`doc_conflict` guidance gains the key-create case as a second worked
example beside whisper-detail, so the deliberately dropped `api` and
`pipeline` flags are not "restored" on a later sync.

SPEC §7.1 generalises the mirroring sentence and adds the fallback chain
and the conditional constraint; §8.3/§8.5 record that `doc_conflict` now
suppresses param drift too, and that a mirrored parameter is not drift.
README gains one line: the org_id fallback, in the resolution-order
section. Per-endpoint flags stay out of it -- below its altitude.

The exit-code fix needs no doc change: SPEC §5.4 and the README table
always specified the correct behaviour, and the fix aligned the
implementation to them. A spec describes the contract, not its
regressions.
The build-sequence plan has served its purpose now that the CLI is
implemented; SPEC.md remains the living specification. Removing it leaves
six dangling citations, all fixed here so nothing points at a file that
no longer exists:

- README dropped the link to it from the closing "see also" line.
- pyproject, model.py (x3) and output.py cited section labels (R3, §1,
  §2, M1.6) that existed only in the plan. The surrounding prose stands
  on its own, so the parentheticals are simply removed -- the P1-P12
  pattern framework they referenced is defined inline in model.py's own
  docstring, so nothing is lost.
- test_model.py's module docstring opened by quoting the plan ("§2 calls
  this the load-bearing design work"); reworded to state the point
  directly.

Verified: `grep -rn IMPLEMENTATION_PLAN` returns nothing. Tests, ruff and
mypy all green.
A human-runnable end-to-end scenario for the `unstract` CLI: extract from
an invoice, build a Prompt Studio project, export it as a tool, deploy an
API, and run it -- with setup (temp config) and teardown (delete created
resources) steps and a summary-report checklist. This is the same flow
whose friction was captured in the recent gotcha-fix pass.
The install step ran `uv pip install --system -e ".[dev]"`, which targets
the runner's `/usr` interpreter. `setup-uv` with `python-version` also
provisions a project venv and exports `VIRTUAL_ENV`, and that interpreter
is externally managed (PEP 668), so `--system` aborts with "externally
managed environment" before any test runs.

Switch to the project workflow the README documents for CI:
`uv sync --frozen --extra dev` installs the exact hash-pinned tree from
uv.lock into the venv (and fails if the lock is stale), and every step
runs through `uv run`. Reproduced green locally: sync, ruff, mypy, pytest
(245), and all three `--discover` index checks.
)

_DOCS = "unstract-docs/docs/unstract_platform/api_documentation/versions"
_BASE = "/api/v1/unstract/{org_id}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintenance-cost flag (for discussion). Worth being explicit about what this base path commits us to.

/api/v1/unstract/{org_id}/... is not a versioned public API — it's the same Django urlconf (backend/urls_v2.py) the React frontend calls, and v1 is just the PATH_PREFIX env-var string. No version negotiation, no deprecation policy, and no published OpenAPI schema (drf-yasg is installed but doesn't serve JSON and isn't mounted in the tenant urlconf). So all 125 platform records are hand-encoded against routes that can move in any normal feature PR — workflow/endpoint/ already shows a land→revert→re-land in git history — and there's nothing to CI-check them against.

Highest-leverage fix: publish a real OpenAPI schema from the backend (add drf-spectacular, mount it inside the tenant urlconf, commit the generated openapi.json). That one change makes drift-detection mechanical, lets CI gate path changes, and opens the door to generating these records instead of hand-maintaining them.

Cheaper interim step: a backend CI job that diffs ALL_ENDPOINTS against django.urls.get_resolver() — ~30 lines, catches renamed/removed routes immediately.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up — how this is trending in 2026, and the tooling that removes the hand-maintenance entirely.

If/when the backend publishes an OpenAPI schema (the suggestion above), this whole registry becomes generated rather than hand-written. Several vendors now generate a full agent-friendly CLI straight from a spec — typed subcommands, --help as JSON, pre-flight validation, dry-run — i.e. exactly the feature list this PR built by hand:

The broader pattern these have converged on for agent-facing CLIs, and where we stand:

  1. Generate from the spec, don't hand-write — the single biggest divergence here; we built by hand only because there's no schema to generate from yet.
  2. Machine-readable discovery + stable exit codes + JSON errors — this PR already nails this (--discover, exit 2/7/9, error envelopes). Speakeasy goes one further with an agent mode that auto-detects Claude Code / Cursor / Codex / Copilot from the environment and adjusts output — cheap to steal.
  3. Ship a skill / AGENTS.md with the CLI — also already done here (bundled update-unstract-cli skill). Ahead of the curve.
  4. CLI and MCP, not either/or — CLI for shell-outable/composable work, MCP for OAuth / audit / multi-tenant / hosted access. The MCP arm is already in flight (feat(mcp): host deployment-scoped and organization-scoped MCP servers unstract#2207), so the hybrid is on track — the one thing to keep aligned is which operations are canonical across the CLI registry and the MCP tool set so coverage doesn't silently diverge.

Net: architecture and agent-ergonomics here are ahead of most vendor CLIs. The schema is the piece that turns items in (1) from maintenance into generation.

Comment thread pyproject.toml
]

[project.scripts]
unstract = "unstract_cli.__main__:main"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binary-name collision — needs an action before publishing. This registers a console script named unstract, but the already-published unstract-client package registers the same name (unstract = "unstract.cli:main", https://github.com/Zipstack/unstract-python-client/blob/main/pyproject.toml), and it already ships a working unstract clone subcommand. Installing both in one env → one silently shadows the other. Flagging so we decide how to handle before PyPI publish (not proposing a fix here).

def _resolve_config(ctx: click.Context, kwargs: dict[str, Any], endpoint: Endpoint) -> ResolvedConfig:
overrides: dict[str, Any] = {}
if base_url := kwargs.get("base_url"):
overrides[f"{endpoint.product.value}.base_url"] = base_url

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: --base-url / --org-id are silent no-ops on all 131 Document Studio commands. The override is stored keyed by product (docstudio.base_url) here, but build_url reads it by API groupconfig.get(endpoint.api, ...)platform.base_url (http.py:135). The keys never match, so it falls through to DEFAULT_BASE_URLS["platform"]. An on-prem/EU user passing --base-url sends their platform key + documents to us-central.unstract.com. Works on whisper/apihub only because there product == api group.

The one covering test (test_cli.py:156) passes a --base-url value identical to the default, so it passes whether or not the flag works. Fix: key overrides by endpoint.api.value.

PROJECT_CONFIG_NAME = ".unstract.toml"


def find_project_config(start: Path | None = None) -> Path | None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (security): project-config discovery is a credential-exfiltration vector. This searches upward from cwd for .unstract.toml ahead of the user's XDG config, with no trust check and no diagnostic naming the file. Combined with env:VAR indirection, a repo-committed .unstract.toml can set base_url to an attacker host and api_key = "env:AWS_SECRET_ACCESS_KEY"; the next unstract run inside that tree ships the env var as a Bearer token to the attacker. Agent sandboxes cd into untrusted checkouts routinely.

Fix: refuse base_url/api_key from project-level files (or make project discovery opt-in), and always print the resolved config path to stderr.


#: Parameters shared by the extraction endpoint. Kept as a module constant so the
#: `--wait` flow and the raw command describe exactly the same surface.
_EXTRACT_PARAMS: tuple[Param, ...] = (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (data loss): whisper extract advertises --save but never defines it. This endpoint is poll.one_shot=True, so generate.py:222 prints "ONE-SHOT: … Use --save to persist it" and errors.py:268 repeats it on exit 9 — but _EXTRACT_PARAMS has no save param, so whisper extract --wait --save out.jsonError: No such option '--save'. The extracted text is one-shot and unrecoverable. It's the only one_shot endpoint missing save (deployment.run has it).

Fix: add Param("save", client_side=True, ...); add a test asserting every one_shot endpoint exposes --save.

_ep("delete", "DELETE", "/workflow/{id}/", "Delete a workflow.", (_WF_ID,),
subgroup="workflow", doc="v1-workflows.mdx", permission=Permission.FULL_ACCESS,
description=f"{_DELETE_NOTE} Only the workflow owner may delete."),
_ep("execute", "POST", "/workflow/execute/", "Execute a workflow.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: workflow execute --file silently drops the entire JSON body. This record is body=BodyKind.JSON but files is ParamLocation.FORM. build_request populates both json= and files=, and httpx sends only files — so workflow_id (required) never reaches the wire, and --dry-run prints the body that gets discarded.

Fix: make this record MULTIPART with all fields FORM, and have build_request raise if more than one body channel is populated rather than letting httpx silently pick one.

def raise_for_status(response: Response, endpoint: Endpoint | None = None) -> None:
"""Convert an unsuccessful -- or deceptively successful -- response into an error."""
# Checked before the status check: this arrives as a 200.
if _looks_consumed(response.payload):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (data loss): a successful result can be destroyed as "already consumed". _looks_consumed substring-matches payload["message"] on every response, before the status check. But for deployment status the message field is the result (per the PollSpec note in deployment.py), so a 200 whose result text happens to contain e.g. "already delivered" raises exit 9 and drops the payload.

Fix: gate this on response.status >= 400 and require message to be a plain string. (Related: poll.py:118 discards response.payload on every non-terminal poll — retain last_payload so an unrecognised terminal status on a one-shot store isn't lost.)

def execute(
plan: RequestPlan,
*,
endpoint: Endpoint | None = None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-idempotent POSTs are retried. execute() accepts this endpoint param but never reads it; transport errors and 5xx are retried for every method, so a read timeout on deployment run / whisper extract re-uploads the document up to 4× → duplicate executions and billing. The docstring only disclaims 4xx. Fix: use the endpoint you already have — force max_retries=0 for one-shot/POST, or retry transport errors only for GET/HEAD.

return min(2.0**attempt, 30.0) * (0.5 + random.random() / 2)


def _retry_after(response: httpx.Response) -> float | None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry-After is honoured with no cap. Retry-After: 3600 → CLI sleeps an hour per attempt, silently and uninterruptibly; under --wait an agent just sees a hang. Fix: clamp to a ceiling (~60s) and to the remaining --wait-timeout budget, and emit a diagnostic on each retry.

hint=f"Pass {param.cli_flag}, or configure it in your profile.",
)
placeholder = "{" + param.name + "}"
path = path.replace(placeholder, str(param.to_wire(value)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path params are interpolated without percent-encoding. --api-name '../../../v1/admin' traverses; --api-name 'x?a=b' injects a query string. Fix: urllib.parse.quote(value, safe=""), with an explicit exemption for share --resource whose enum values intentionally span path segments.

plan = http.build_request(endpoint, config, values)

if kwargs.get("dry_run"):
emit(plan.describe(), fmt if fmt is not OutputFormat.RAW else OutputFormat.JSON)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--dry-run output isn't scrubbed with plan.secrets. plan.describe() redacts only by header/key name; plan.secrets (populated for exactly this) is used only on the error path. FORM-located JSON params are re-serialised to a string out of redact_value's reach, so deployment run --custom-data '{"api_key":"sk-…"}' prints the key verbatim while the same JSON in a BODY param is redacted. Fix: scrub(render(...), plan.secrets) before emitting.

try:
plan = http.build_request(endpoint, resolved, {})
response = http.execute(plan, endpoint=endpoint, max_retries=0)
except Exception as exc: # noqa: BLE001 - doctor must never itself crash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config doctor can't be a CI gate, and _probe leaks the key. doctor always exits 0 even when every credential is unresolved / every live check fails. And this except Exception returns str(exc) unscrubbed to stdout (e.g. "upstream rejected key SUPERSECRET…") while every other error path calls scrub(). Fix: exit non-zero when a probed group fails or a required api_key is unresolved; scrub(str(exc), collect_secrets(...)) and narrow the catch.

# Click returns whatever `ctx.exit` was given; a command that simply
# falls off the end returns None, which is success.
return result if isinstance(result, int) else 0
except click.UsageError as exc:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-Click exceptions bypass the structured-error contract. Only Click exceptions are caught here; a KeyError from a bad PollSpec name (get_endpoint) or a yaml.safe_dump on bytes exits with a bare traceback and no JSON envelope — breaking the guarantee advertised to agents in app.py:396. Fix: add a final except Exception emitting a GENERIC CLIError envelope.

return resolved


def _save_payload(payload: Any, destination: str, raw_field: str | None) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrency hazards for parallel agents. (1) --save writes a deterministic .{name}.partial temp file at default umask — two agents saving to one path corrupt each other, and the file lands 0644 while the config path goes to lengths for 0600. Use tempfile.mkstemp(dir=path.parent) + os.replace + chmod 600. (2) config set (loader.py:215) is a non-atomic O_TRUNC read-modify-write with no lock, and rebuilds the doc from known keys only — it silently drops any top-level table it doesn't recognise. Use temp-file + os.replace and preserve unknown keys.

subgroup="group member", body=BodyKind.JSON, doc="v1-user-groups.mdx",
permission=Permission.READ_WRITE),
# No trailing slash on this path, unlike its siblings.
_ep("remove", "DELETE", "/groups/{id}/members/{user_id}", "Remove a member from a group.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no_trailing_slash=True is wrong for this route. The backend router is DefaultRouter(trailing_slash=True) (tenant_account_v2/groups_urls.py), so this DELETE requires the slash; it only works today because follow_redirects=True follows Django's APPEND_SLASH 301 (extra round-trip, breaks if redirects are ever disabled). The module docstring's claim that these paths "genuinely have none" is false for this one — the other nine no_trailing_slash records verified correct against the live URL resolver. Fix: drop no_trailing_slash=True here.

Comment thread tests/test_gotchas.py
@@ -0,0 +1,245 @@
"""Regressions for the friction points recorded in GOTCHAS.md.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (tests + comments, batched to keep noise down).

Tests: coverage is 88% but ~11/20 targeted mutations survive — the endpoint records and config-resolution are largely asserted against themselves (changing a record's path, deleting the require_response_fields guard, or swapping env/profile precedence all keep the suite green). Highest-value adds: a --dry-run smoke test walking every endpoint (assert URL has no unsubstituted {...} + auth header present — would have caught the --base-url blocker); a precedence test for flag>env>profile>default; and actually running the docdiff drift suite in CI (those 4 tests are the only drift protection and currently skip because CI checks out only this repo). Also uv run pytest fails on a fresh checkout (ModuleNotFoundError: tomli_w) — needs --extra dev; README:263 documents the bare command that doesn't work.

Comments: ~57 citations to GOTCHAS #N / CAPTURE2 … / BUG N / DOC N — none of those files exist in the repo, and 19 render into user-facing --help. Either commit those docs or inline the fact at the use-site (as was already done well for the P1–P12 table). Also RequiredUnless (model.py:179) docstrings a "live case" that doesn't exist and is unused in src/; and platform.py:142 says "Verified this session". The load-bearing defect comments (poll.py:6, http.py:428) are genuinely good — keep them, just reword "currently returns X" → "insensitive to X" so they don't rot once the backend is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants