fix(v2): let an explicit options.password win over the password shorthand - #121
Merged
Merged
Conversation
…hand
`password` is shorthand for the contract field `options.password` -- the spec
declares the password inside `options` and nowhere else. `buildParseForm`
spread the shorthand in last, so it overwrote a password the caller had written
into `options`, while ade-python's `_build_parse_body` resolves the same
conflict the other way. The same call therefore decrypted with a different
password depending on the language, and the losing one surfaced only as a 422
`encrypted_pdf_wrong_password` naming no cause.
Give the explicit field precedence, matching ade-python, and test the value
rather than key presence: `{password: undefined}` is how JS spells "absent" and
`JSON.stringify` drops the key, so a presence test would suppress the shorthand
and then erase the key, sending a locked PDF with no password at all.
Also drop the fallback that wrote a top-level `password` form field when
`options` was a string that did not parse -- the contract declares no such
field, so the gateway dropped it and the key was lost silently. `options` now
goes through one helper that rejects a non-object on both the string and the
object branch, mirroring `coerceSchema`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently handles both parse routes and is well covered by focused tests.
Pull request overview
Aligns V2 parse password precedence with the Python SDK while validating serialized options.
Changes:
- Makes explicit
options.passwordoverride shorthandpassword. - Rejects malformed or non-object options.
- Adds documentation and regression tests.
File summaries
| File | Description |
|---|---|
src/resources/v2/parse.ts |
Implements precedence and options validation. |
tests/api-resources/v2/v2.test.ts |
Covers serialization, precedence, and invalid inputs. |
README.md |
Documents password behavior. |
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.
…tened paths Review follow-ups on the precedence fix. `opts?.['password']` walks the prototype chain, so a polluted `Object.prototype.password` made every options object look like it already carried one: the shorthand suppressed, nothing serialized, and a locked PDF shipped with no password at all. Read it as an own property. The rest is coverage and wording for behavior the first commit changed but did not pin: an explicit `options.password: null` silencing the shorthand, a malformed `options` rejected on a call with no password in it (`options` is coerced unconditionally now), and the object branch rejecting an array. Both READMEs now state the `null` rule and the coercion, `Unsupported options type` names an array as `array` rather than `object`, and the rejection cases are table-driven so one bad input no longer hides the rest. Also fixes tests that pinned `pages: '1-2'`, which the spec types `integer[] | null` under `additionalProperties: false` -- they documented a request the gateway rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 9, 2026
tian-lan-landing
added a commit
to landing-ai/ade-python
that referenced
this pull request
Sep 9, 2026
…ct options (#160) * docs(v2): pin and document the parse password precedence rule `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> * fix(v2): reject an options value that is not a JSON object `_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> * docs(v2): document the options tightening and cover its untested paths 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> * fix(v2): type options as accepting a JSON string, and name both rejection 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing
added a commit
to landing-ai/ade-python
that referenced
this pull request
Sep 9, 2026
…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>
tian-lan-landing
added a commit
that referenced
this pull request
Sep 9, 2026
…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>
tian-lan-landing
added a commit
to landing-ai/ade-python
that referenced
this pull request
Sep 9, 2026
…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>
tian-lan-landing
added a commit
to landing-ai/ade-python
that referenced
this pull request
Sep 9, 2026
…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>
tian-lan-landing
added a commit
that referenced
this pull request
Sep 9, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
passwordis shorthand for the contract fieldoptions.password. This SDK let the shorthand win a conflict; ade-python lets the explicit field win. Same call, different password, depending on the language — the loser surfaced only as a 422encrypted_pdf_wrong_passwordnaming no cause.Aligns on
options.passwordwins. Paired with landing-ai/ade-python#160, which documents the same rule and pins it with tests.{password: undefined}is JS for "absent" andJSON.stringifydrops the key, so a presence test would suppress the shorthand and erase the key, sending a locked PDF with no password at allpasswordform field — the spec has declared no such field since 2026-07-16, so the gateway dropped it and the key was lost silentlyoptionsnow goes through one helper rejecting a non-object on both the string and object branch, mirroringcoerceSchemaFixes:
🤖 Generated with Claude Code