Re-vendor the gateway schema and validator bundle: OAuth scopes are optional - #42
Re-vendor the gateway schema and validator bundle: OAuth scopes are optional#42oschaaf wants to merge 2 commits into
Conversation
|
Nice piece of work — especially finding and fixing the constraint bug while adding the new kind. Reviewed alongside the product-side schema change and the gateway-side DCR change. I verified the re-vendor claim independently rather than trusting the description: I also probed So the union reorder plus One thing worth checking before the hold lifts, and one carry-over. The hold should name the gateway version, not just the schema deploy
That covers the static-credential OAuth shape, but not the DCR / issuer-only one. Once the schema change is live, an OAuth server authored without scopes emits an explicit empty list, and on the gateway side This matters here because the skill and the harness both learn from this change that a scopeless OAuth block is fine, and the skill can produce an Two options: extend the hold to "schema change deployed and gateways on ≥1.0.7.1", or add a clause to the That second point is worth making regardless of timing — the product now always emits the key, so the DCR default becomes unreachable for these configs. The current guidance only covers the "provider rejects any scope" case, which is a different situation from "I'm using DCR and didn't think about scopes." Carry-over: the vendored bundle still inlines the two product-template comment linesYou flagged this in the description as a product-side follow-up, and it is pre-existing rather than introduced here — but this commit does re-publish it, so it's worth opening the follow-up now rather than leaving it in a PR body. Stripping comments in the bundle build (or extending the leak guard to cover them) would close it at the source, and the same guard would catch the next one. CoverageThe harness tests are the strongest part of this. The digest generator's shift from a hardcoded field list to "preferred order, then everything else in artifact order" is the right fix for the coverage gate — I checked the rendered OAuth table and it picked up only the two genuinely user-facing fields, with none of the lower-level OAuth knobs leaking into the customer-facing digest. |
…ptional
`mcp_servers[].authentication.scopes` (type: oauth) no longer has to carry
an entry. Some OAuth providers reject ANY `scope` parameter with
`invalid_scope` (Docebo is the public example); with `scopes` omitted or
`[]` the gateway now sends no `scope` parameter at all. The artifact
reports every field with a schema default as `required: false`, which
also flips `gateway.authentication.enabled` (default `true`) to optional
and drops `scopes` from the oauth variant's requiredFields.
Vendored from product-repo revision 9e37ef5c (the change that made OAuth
scopes optional), both files byte-copied together:
- dtwo/skills/dtwo-gateway-config/schema-reference.json — generatorVersion
1.1.0, sha256 22f5fa77bd79b0686b23f663756e787613547f2f2d65e8e98d2a5d012ec0e30e
- skill-harness/vendor/config-validator.bundle.mjs — bundle 4.0.0, sha256
d286b15fd04cb5a782fad26ed797a6347c0fbb00eef987355812853ba44488a0,
verified against that revision's committed validator-bundle.sha256
sidecar before copying. The .d.mts shim is byte-identical to the old one.
The previous vendor (2026-08-20) lagged the product repo, so this refresh
also carries what landed upstream since, none of it scopes-related:
`token_endpoint_auth_method` and `omit_resource` on the oauth variant
(the bundle rejects an unimplemented auth method and `client_secret_basic`
without a `client_secret`), `session_control.redirect_uri` plus an
optional `session_control.client_id` with two new cross-field
constraints, refreshed clearing/CGNAT guidance, and six more reserved
`gateway.advanced` keys.
Digest generator: the OAuth variant table was a hardcoded field list with
no Default column, so the coverage gate refused the new artifact (the two
new oauth fields rendered nowhere; `scopes`' `[]` default had no cell).
The table is now ordered-then-data-driven and routes its Default column
through defaultCell; the hand-written "both shapes still require ...
scopes" sentence — and the per-variant table outside the generated block
in SKILL.md — now say scopes is optional and when to leave it out, with
one clause on the DCR path: an issuer-only block with `scopes` omitted
registers the client with no scopes at all; the gateway's own DCR default
does not apply to configs authored here. The bundle also rejects a
singular `scope` key on an oauth block ("did you mean scopes") instead of
stripping it silently.
Pins: EXPECTED_SCHEMA_ARTIFACT_SHA256, EXPECTED_VALIDATOR_BUNDLE_VERSION
(2.0.0 -> 4.0.0) and EXPECTED_VALIDATOR_BUNDLE_SHA256 updated together.
The three GATEWAY_OWNED_SAFE_DEFAULTS rationales are unchanged in the
refreshed artifact (re-verified per README).
known-defects.md: the re-vendor log sits on the resolved entry 4; the
stale duplicate open copy of that entry is removed.
Plugin version 1.1.7 -> 1.1.8: SKILL.md content ships to customers.
Hold: merge only after the product schema change is deployed AND
gateways run 1.0.7.1 or later. Until the schema deploys the harness would
accept scopeless OAuth configs the live product still rejects; and on the
issuer-only/DCR shape the explicit empty scopes list the product now
always emits makes pre-1.0.7.1 gateways send an empty `scope` string in
the RFC 7591 registration request instead of falling back to their DCR
default.
Signed-off-by: Otto van der Schaaf <otto@dtwo.ai>
…add a no-scopes OAuth fixture
Adding a `max_length` value-constraint kind exposed that `regex` and
`min_length` had never been enforced. `ValueConstraintSchema` listed the
equals-constraint (`equals: z.unknown()`) first in the zod union;
`z.unknown()` accepts an absent key and a plain object strips unknown
keys, so `{ path, regex }` and `{ path, min_length }` both parsed to a
bare `{ path }`, `constraintKind` fell through to min_length with an
undefined bound, and `len < undefined` never fails. 37 regex and 12
min_length assertions across the battery were vacuous; `equals`,
`required_paths` and `forbidden_paths` were unaffected. Recorded as
known-defects.md entry 5 (`value-constraints-stripped-at-parse`), with
the caveat that past bench passes on those assertions are unproven.
Fix: specific kinds match first, every member is a strictObject (a
misspelt key is now a fixture-load error), the equals-constraint refines
on key presence, and fixtures.test.ts asserts every constraint in the
battery keeps exactly one kind key after parse (>= 100, anti-vacuity).
`max_length` mirrors `min_length` except that an absent path counts as
length 0, so `max_length: 0` reads "absent or empty". `forbidden_paths`
cannot express that for `scopes`: on a validating config parseConfig
fills the defaulted `scopes: []` in, so the key is present whether or
not the YAML carried it.
New aspirational fixture `docebo-oauth-no-scopes`: authorization_code
with a static client triple, authorize/token on the generic
`/oauth2/authorize` + `/oauth2/token` shapes at placeholder domains, and
`scopes` asserted absent-or-empty because the provider rejects any
`scope` parameter. Its must_validate assertion needs the 3.0.0 validator
bundle from the previous commit.
Battery: 12 required + 18 aspirational; README tables and counts updated.
Signed-off-by: Otto van der Schaaf <otto@dtwo.ai>
pareilly
left a comment
pareilly
left a comment
There was a problem hiding this comment.
These and my changes will have conflicts because they both are taking changes from the same sources. We will probably want mine to go in first and then rebase yours.
However, they both have to wait until after we deploy to prod on Thursday
|
Thanks — both points taken, and the probe you ran is exactly the assurance I wanted on the union fix.
Rebuilt commits: 1dab19c (re-vendor, held) + c57ad74 (harness). 186/186 tests, digest in sync. |
30af3f6 to
c57ad74
Compare
|
Re-reviewed at Verified against the product head, not the descriptionAll four pins agree exactly, and the vendored bytes hash to their own pins:
I also probed the newly vendored bundle through its own seam rather than trusting the log line: So the typo guard is live at the harness seam, and the DCR clause propagated to all three places in the skill — the generated Still open: the collision with #40The update didn't touch this, and it's the one thing I'd want settled before either merges. #40 branches from the same commit as this one and neither contains the other, so whichever lands second inherits the conflict. Both still bump Four files conflict on a test-merge. Three resolve mechanically to this branch's side, because its vendored artifacts strictly supersede #40's — I diffed them, and every session-control /
The fourth needs a decision, not a resolution.
The consequence is real: with auto-append, the "every user-audience field with a target must render" clause of One more sequencing noteThis branch's hold now names the schema deploy and 1.0.7.1. But the re-vendored digest also carries the rewritten Landing #40 first and rebasing this onto it keeps that honest, and the rebase is mostly "take ours" per the table above. The reverse order works too, but means deleting most of #40 and bypassing its gate. Carry-overThe two product-template comment lines in the vendored bundle are unchanged — still worth opening that product-side follow-up (strip comments in the bundle build, or extend the leak guard) rather than leaving it in a PR description, since each re-vendor re-publishes them. |
|
Agreed on all counts, and thanks for the test-merge against #40 — that saved the guesswork. Order: #40 first, then this branch rebases onto it and bumps The Sequencing note: understood — merging this first would ship #40's gated Heads-up on the pins: dtwoai/d2#2051 is getting one more amend from its own review (the Carry-over: filed as dtwoai/d2#2052 (strip template comments in the bundle build, extend the leak guard). |
2 participants
Why
The gateway config schema is making
mcp_servers[].authentication (oauth).scopesoptional with an explicit empty-list default. Some OAuth providers (Docebo is the public example) reject anyscopeparameter withinvalid_scope; with no scopes configured the gateway sends noscopeparameter at all. This repo still says scopes are mandatory in three places, and both vendored artifacts predate the change.What changes
scopesrow (optional, default[], new guidance text), thegateway.authentication.enabledrow's Required flag (defaulted fields now reportno), and the oauth variant's required-field list dropsscopes. The previous vendor dated from 2026-08-20, so the copy also picks up everything the product merged since (OAuthtoken_endpoint_auth_methodandomit_resource, session-controlredirect_uriand optionalclient_id, the CGNAT allowed-networks guidance, six more reserved keys). Verbatim copies cannot be split, so this is one commit that can be dropped whole.dtwo-gateway-config/SKILL.mdno longer claimsscopesis required; it now says to omit it (or set[]) for providers that reject any scope.max_lengthconstraint kind (an absent path counts as length 0) so a fixture can assert "no scopes or an empty list".equals: z.unknown()first, so everyregexandmin_lengthconstraint parsed to a bare{path}and never fired. Fixed with strict objects and specific kinds first; recorded in known-defects. Expect some aspirational pass rates to drop on the next live run, since 49 assertions start counting.plugin.jsonversion bump (SKILL.md content ships to customers).Follow-up (product side, not this PR)
The vendored bundle inlines two product-template comment lines that name an internal design record. They were present in the previous bundle too; the product's bundle build should strip comments or extend its leak guard.
Gates
skill-harness:pnpm test186 passed / 0 failed (31 suites, including the digest drift check and both artifact sha pins);pnpm biome:checkclean.node scripts/generate-schema-digest.mjs --checkin sync.[], and a barescopes:all parse to[];scopes: [""]is rejected.