fix(v2): pin the parse password precedence rule and reject a non-object options by tian-lan-landing · Pull Request #160 · landing-ai/ade-python

tian-lan-landing · 2026-09-09T01:54:09Z

password is shorthand for the contract field options["password"]. This SDK already let the explicit field win; ade-typescript let the shorthand win — same call, different password, depending on the language. Aligned on this SDK's rule; ade-typescript moves to match in landing-ai/ade-typescript#121.

9c3883e — docs + tests, no behavior change. Pins the rule on the two paths that had no conflict coverage (options as a JSON string, and parse_jobs.create), and documents password in the README, which had no encrypted-PDF section at all.

ba6e807 — one real bug found while doing that. dict(json.loads(...)) accepts any pair-sequence, so a JSON array silently became the options dict:

options='[["password", "sneaky"]]', password="kw"  ->  {"password": "sneaky"}

The smuggled key then beat the caller's own password — the exact silent substitution the precedence rule exists to prevent. options now goes through _coerce_options, mirroring coerce_schema_to_dict.

Fixes:

https://app.asana.com/1/504311096896991/project/1208747057905216/task/1218273814950544

🤖 Generated with Claude Code

`password` is shorthand for the contract field `options["password"]`, and `_build_parse_body` already lets the explicit field win the tie. That rule lived only in a comment and a docstring, and only the sync route's dict branch had a test for it -- ade-typescript had shipped the opposite rule, so the same call decrypted with a different password depending on the SDK. - pin the rule on the `options` JSON-string branch and on `parse_jobs.create`, the two paths that had no conflict coverage - state the cross-SDK contract next to the code that implements it, replacing a vaguer note that read as being about folding rather than precedence - document `password` and the rule in the README, which had no encrypted-PDF section at all No behavior change; ade-typescript moves to this rule to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`_build_parse_body` coerced `options` with `dict(json.loads(...))`, and `dict()` accepts any pair-sequence -- so a JSON *array* silently became the options dict: options='[["password", "sneaky"]]', password="kw" -> {"password": "sneaky"} The smuggled key then won the precedence tie against the caller's own `password` argument, which is the exact silent substitution that rule exists to prevent. Non-dict scalars fared no better: `TypeError: 'int' object is not iterable`, or a `ValueError` about a "dictionary update sequence" -- neither naming the field. Route `options` through `_coerce_options`, mirroring `coerce_schema_to_dict` in `lib/schema_utils.py`: decode a string, require an object, and name the field when it is not one. Malformed JSON still surfaces as the `ValueError` that `json.loads` raises, exactly as it does for `schema`. Found while aligning precedence with ade-typescript, which rejects the same inputs (landing-ai/ade-typescript#121). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The implementation consistently applies the documented rule across shared sync, async, and job request construction.

Pull request overview

Clarifies and enforces V2 parse password precedence while rejecting invalid non-object options.

Changes:

Adds strict options coercion.
Tests sync and job precedence behavior.
Documents encrypted PDF handling.

File summaries

File	Description
`src/landingai_ade/resources/v2/parse.py`	Validates options and documents precedence.
`tests/api_resources/v2/test_parse.py`	Adds regression and validation tests.
`README.md`	Documents encrypted PDF passwords.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Review follow-ups on the precedence work. `_coerce_options` narrowed the non-string branch from `dict()` to `Mapping`, not just the JSON-string branch -- and that half was undocumented and untested. It is deliberate: `dict()` accepts any pair-sequence in list form too, so `[["password", "x"]]` smuggled a password exactly like its string twin did. The docstring now says so, including the one place this stops mirroring `coerce_schema_to_dict` (that helper takes a pydantic model; `options` never advertised one), and the rejection test covers both forms plus the previously uncovered `Unsupported options type` branch. Also: drop the `str` arm of the final serialization, which `_coerce_options` made unreachable while still reading as "a pre-serialized string is forwarded verbatim" -- the opposite of what the code does; add async precedence coverage, since all four call sites share `_build_parse_body` but only the sync two were pinned; make the rejection cases table-driven so one bad input no longer hides the rest; and fix a README example that imported `os` but used an undefined `Path`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Public annotations exclude documented string options, and malformed-JSON exceptions are documented incorrectly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/landingai_ade/resources/v2/parse.py:270

Malformed JSON does not raise TypeError: _coerce_options lets json.loads raise json.JSONDecodeError, and the new test explicitly asserts that behavior. This exception promise is therefore inaccurate; distinguish malformed JSON from decoded non-objects.

              field. Must be a mapping, or a JSON string that decodes to an object --
              anything else raises `TypeError` before the request is sent.

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Balanced

…tion errors Copilot review on #160. The docs this branch added advertise a JSON string for `options`, and `_coerce_options` accepts one, but every public annotation still said `Optional[Mapping[str, object]]` -- so a typed caller could not use the advertised form without the `type: ignore` the tests carried. Widen all six sync/async parse and job signatures, matching how extract already types its coercible sibling (`schema: Union[str, Mapping[str, object], Type[BaseModel]]`), and drop the ignores that are no longer needed. griffe reports no breakage: the change is additive. The same docs also promised `TypeError` for anything unacceptable, which is wrong for the case most likely to hit it -- malformed JSON propagates `json.JSONDecodeError` from `json.loads`, as this branch's own test asserts. Name both paths instead, in the two docstrings and the README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The implementation, public annotations, documentation, and tests consistently enforce the stated contract.

Review details

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Balanced

…e aliases check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring prompt scoped routes too, and said nothing about which FIELDS back a /v2 request — which is how the parse password went wrong: the spec has declared it at options.password and nowhere else since 2026-07-16, the same spec still declares a top-level `password` on /v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere. They picked opposite ones, so one call sent a different password per language (#160, landing-ai/ade-typescript#121). - the prompt's SCOPE block now scopes fields as well as routes, names both traps (a nested spec field is not a top-level one; a /v1 top-level field is not a /v2 field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one. - CONTRIBUTING records the two aliases that exist (`password` -> options.password, `strict` -> options.strict), the precedence rule for each, the language-specific traps in both SDKs, and — plainly — that nothing in CI checks any of it, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand. A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was built and dropped as more machinery than the problem warrants: it could not verify a tie-break from code anyway, only that a rule had been written down somewhere. The risk stays with review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…e aliases check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring prompt scoped routes too, and said nothing about which FIELDS back a /v2 request — which is how the parse password went wrong: the spec has declared it at options.password and nowhere else since 2026-07-16, the same spec still declares a top-level `password` on /v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere. They picked opposite ones, so one call sent a different password per language (#121, landing-ai/ade-python#160). - the prompt's SCOPE block now scopes fields as well as routes, names both traps (a nested spec field is not a top-level one; a /v1 top-level field is not a /v2 field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one. - CONTRIBUTING records the two aliases that exist (`password` -> options.password, `strict` -> options.strict), the precedence rule for each, the language-specific traps in both SDKs, and — plainly — that nothing in CI checks any of it, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand. A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was built and dropped as more machinery than the problem warrants: it could not verify a tie-break from code anyway, only that a rule had been written down somewhere. The risk stays with review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…e aliases check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring prompt scoped routes too, and said nothing about which FIELDS back a /v2 request — which is how the parse password went wrong: the spec has declared it at options.password and nowhere else since 2026-07-16, the same spec still declares a top-level `password` on /v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere. They picked opposite ones, so one call sent a different password per language (#160, landing-ai/ade-typescript#121). - the prompt's SCOPE block now scopes fields as well as routes, names both traps (a nested spec field is not a top-level one; a /v1 top-level field is not a /v2 field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one. - CONTRIBUTING records the two aliases that exist (`password` -> options.password, `strict` -> options.strict), the precedence rule for each, the language-specific traps in both SDKs, and — plainly — that nothing in CI checks any of it, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand. A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was built and dropped as more machinery than the problem warrants: it could not verify a tie-break from code anyway, only that a rule had been written down somewhere. The risk stays with review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…e aliases (#161) check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring prompt scoped routes too, and said nothing about which FIELDS back a /v2 request — which is how the parse password went wrong: the spec has declared it at options.password and nowhere else since 2026-07-16, the same spec still declares a top-level `password` on /v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere. They picked opposite ones, so one call sent a different password per language (#160, landing-ai/ade-typescript#121). - the prompt's SCOPE block now scopes fields as well as routes, names both traps (a nested spec field is not a top-level one; a /v1 top-level field is not a /v2 field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one. - CONTRIBUTING records the two aliases that exist (`password` -> options.password, `strict` -> options.strict), the precedence rule for each, the language-specific traps in both SDKs, and — plainly — that nothing in CI checks any of it, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand. A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was built and dropped as more machinery than the problem warrants: it could not verify a tie-break from code anyway, only that a rule had been written down somewhere. The risk stays with review. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…e aliases (#122) check-v2-paths.sh scopes client.v2 to the spec's /v2 routes. The wiring prompt scoped routes too, and said nothing about which FIELDS back a /v2 request — which is how the parse password went wrong: the spec has declared it at options.password and nowhere else since 2026-07-16, the same spec still declares a top-level `password` on /v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere. They picked opposite ones, so one call sent a different password per language (#121, landing-ai/ade-python#160). - the prompt's SCOPE block now scopes fields as well as routes, names both traps (a nested spec field is not a top-level one; a /v1 top-level field is not a /v2 field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one. - CONTRIBUTING records the two aliases that exist (`password` -> options.password, `strict` -> options.strict), the precedence rule for each, the language-specific traps in both SDKs, and — plainly — that nothing in CI checks any of it, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand. A mechanical gate for this (check-v2-aliases.sh + a shared manifest) was built and dropped as more machinery than the problem warrants: it could not verify a tie-break from code anyway, only that a rule had been written down somewhere. The risk stays with review. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

tian-lan-landing and others added 2 commits September 9, 2026 09:37

Copilot AI balanced review requested due to automatic review settings September 9, 2026 01:54

tian-lan-landing mentioned this pull request Sep 9, 2026

fix(v2): let an explicit options.password win over the password shorthand landing-ai/ade-typescript#121

Merged

Copilot started reviewing on behalf of tian-lan-landing September 9, 2026 01:54 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 9, 2026 04:08

Copilot started reviewing on behalf of tian-lan-landing September 9, 2026 04:09 View session

This was referenced Sep 9, 2026

docs(spec-sync): scope the wiring prompt to request fields, record the aliases #161

Merged

docs(spec-sync): scope the wiring prompt to request fields, record the aliases landing-ai/ade-typescript#122

Merged

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread src/landingai_ade/resources/v2/parse.py

Comment thread README.md Outdated

Comment thread src/landingai_ade/resources/v2/parse.py Outdated

Copilot AI review requested due to automatic review settings September 9, 2026 08:02

Copilot started reviewing on behalf of tian-lan-landing September 9, 2026 08:02 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

tian-lan-landing merged commit af899a6 into main Sep 9, 2026
6 checks passed

Navigation Menu

Uh oh!

fix(v2): pin the parse password precedence rule and reject a non-object options - #160

fix(v2): pin the parse password precedence rule and reject a non-object options#160
tian-lan-landing merged 4 commits into
mainfrom
fix/v2-password-precedence-parity

tian-lan-landing commented Sep 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tian-lan-landing commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tian-lan-landing commented Sep 9, 2026 •

edited

Loading