docs(spec-sync): scope the wiring prompt to request fields, record the aliases - #161
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical validation gaps allow operations and undeclared aliases to bypass the new gate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a cross-SDK gate ensuring V2 convenience parameters align with request-body fields or documented aliases.
Changes:
- Adds a shared alias manifest and validation script.
- Integrates validation into lint and spec-sync workflows.
- Documents alias and precedence rules.
File summaries
| File | Review |
|---|---|
specs/v2-aliases.json |
Defines shared V2 aliases and precedence. |
scripts/spec-sync/check-v2-aliases.sh |
Requires fixes: referenced and empty request bodies can bypass operation validation (critical), and top-level targets bypass precedence validation (moderate). |
scripts/lint |
Runs the new validation. |
CONTRIBUTING.md |
Documents the alias policy. |
.github/workflows/spec-sync.yml |
Integrates the gate into spec synchronization. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tian-lan-landing
added a commit
that referenced
this pull request
Sep 9, 2026
From Copilot review on #161 / landing-ai/ade-typescript#122. - An operation is now recorded from an "O" row, emitted whenever it has a requestBody at all, instead of being inferred from the fields found in it. A body with no top-level properties used to disappear from the operation list entirely, so a NEW /v2 route with an empty or referenced body passed with exit 0 — its missing OPERATIONS mapping and any params it declared went unchecked. - `requestBody` is dereferenced before reading `content`, so a body behind a `$ref` (valid OpenAPI; this generator inlines, but that is not a guarantee) reads the same as an inline one. A `$ref` that does not resolve is reported against the spec — it used to surface as a misleading complaint about the manifest. - A manifest target naming a TOP-LEVEL spec field no longer skips the precedence check. The target is itself the thing a caller sets directly, so it decides whether a conflict is possible: "sole" is now rejected when the SDK declares it. Both JSON readers keep producing identical dumps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tian-lan-landing
added a commit
to landing-ai/ade-typescript
that referenced
this pull request
Sep 9, 2026
From Copilot review on #122 / landing-ai/ade-python#161. - An operation is now recorded from an "O" row, emitted whenever it has a requestBody at all, instead of being inferred from the fields found in it. A body with no top-level properties used to disappear from the operation list entirely, so a NEW /v2 route with an empty or referenced body passed with exit 0 — its missing OPERATIONS mapping and any params it declared went unchecked. - `requestBody` is dereferenced before reading `content`, so a body behind a `$ref` (valid OpenAPI; this generator inlines, but that is not a guarantee) reads the same as an inline one. A `$ref` that does not resolve is reported against the spec — it used to surface as a misleading complaint about the manifest. - A manifest target naming a TOP-LEVEL spec field no longer skips the precedence check. The target is itself the thing a caller sets directly, so it decides whether a conflict is possible: "sole" is now rejected when the SDK declares it. Both JSON readers keep producing identical dumps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The checker has critical operation-identity gaps and additional unresolved coverage issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
specs/v2-aliases.json:34
- At the current heads, this target-wins rule is not yet the shared, tested contract described by the PR: this branch's jobs test only covers folding when
optionsis absent, while the conflict test remains in open #160; paired TypeScript #122 still has the alias-win implementation until open #121 lands. Because this checker explicitly cannot verify the tie-break, it can pass while the stated rule is not enforced. Make #160/#121 merge prerequisites and rebase the gate PRs, or include their fixes and tests before merging.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
tian-lan-landing
force-pushed
the
ci/v2-alias-parity-gate
branch
from
September 9, 2026 09:23
6c03a63 to
9339419
Compare
tian-lan-landing
changed the title
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The Python precedence guidance remains inaccurate and warrants human review.
Review details
Suppressed comments (1)
CONTRIBUTING.md:222
- This instruction is backwards for this Python SDK. To preserve the stated rule that an explicit
Nonewins,_build_parse_bodymust—and currently does—test whether the key is present; a value-based check would treatNoneas absent and overwrite it with the shorthand. Scope the value check to TypeScript’sundefinedsemantics and state the Python rule explicitly.
- `password` on `/v2/parse` and `/v2/parse/jobs` → `options.password`. When a caller supplies both,
an explicit `options.password` **wins**, `null` included (`null` means "no password"). Test the
*value*, not key presence: `undefined` is JS for "absent" and `JSON.stringify` drops the key, so a
presence test would suppress the shorthand *and* send no password at all.
- Files reviewed: 2/2 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>
tian-lan-landing
force-pushed
the
ci/v2-alias-parity-gate
branch
from
September 9, 2026 09:37
9339419 to
d63391c
Compare
Collaborator
Author
|
Branch rewritten: this is now prompt + docs only. The earlier Copilot threads on this PR were about that removed script (and, in ade-typescript, the removed manifest), so they no longer apply to the diff. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Existing alias names and targets should be explicitly deferred to a maintainer.
Review details
Suppressed comments (1)
.github/workflows/spec-sync.yml:524
- The alias rule is incomplete for drift involving an existing alias. “Wire the spec’s own shape” conflicts with “do NOT … re-target an alias” if the spec adds a top-level
password/strict, removes an existing target, or changes its nesting; the AI could either preserve an obsolete alias or send the value twice. Explicitly defer changes involving existing alias names or targets to a maintainer, not only changes that appear to need a new alias.
maintainer's call — do NOT add, remove or re-target an alias. Wire the spec's own
shape, and if a spec change looks like it needs a new one, leave that field unwired
and say so in your summary.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
The wiring prompt scoped
client.v2to the spec's/v2routes and said nothing about which fields back a/v2request. That is how the parse password went wrong: the spec declares it only atoptions.password, the same spec still declares a top-levelpasswordon/v1/ade/parse*, and both SDKs ended up with a hand-written top-level shorthand whose tie-break was written down nowhere (#160 / landing-ai/ade-typescript#121).Prompt and docs only — no code, no new script,
scripts/lintand the CI gates untouched./v1top-level field is not a/v2field), and tells the AI pass to leave a new convenience alias to a maintainer rather than invent one.password→options.password,strict→options.strict), the precedence rule for each, the language-specific traps on both sides, and — plainly — that nothing in CI checks this, so a PR touching a non-spec top-level param has to be diffed against the other repo by hand.Enforcing it mechanically was considered and rejected: a gate can check that a rule has been written down somewhere, not that the code obeys it, and that is not worth the machinery. The risk stays with review.
Paired with landing-ai/ade-typescript#122.