Conversation
Content calls previously requested one service-wide `content` scope, with a path-sniffing special case for the public QuranReflect reads. This adds operation-level scope selection driven by the pinned contract, behind an opt-in flag. `contentScopeMode` defaults to "legacy" and reproduces the previous behaviour exactly, including the /quran-reflect/v1/posts/ path handling that mapped to post.read and comment.read. Existing credentials and existing code are unaffected; a test asserts the legacy token requests are unchanged. Changing the default is a deliberate major-release decision (D6), not something this commit does. In "granular" mode a content call requests the single scope its operation declares. Scope metadata now flows from the contract into the OpenAPI documents (x-qf-scopes) and from there into the generated operation catalog, so the SDK does not re-derive policy from a path or a service name. Quota fields are deliberately left out of the catalog: rate limiting is the gateway's business and must never be an authorization input in a client. Deliberate refusals rather than fallbacks: - An untyped client.fetch() call carries no operation, so granular mode cannot know its scope. It raises an actionable error naming the escape hatches instead of quietly requesting `content`, which would defeat the mode and fail anyway for credentials never granted it. - An insufficient-scope response or an invalid_scope rejection is reported with the requested scopes and is never retried with a broader scope. Widening on failure would paper over a misconfigured client and hand it more access than it holds. - `search` and `analytics.events.write` stay separate permissions and are never requested for a content call, in either mode. Token handling fixes found while doing this: - The cache key was `service:scope`, which reused one issuer's token against another. It now covers the token endpoint, client, audience and canonical scope set, and scope order no longer produces duplicate entries. - Concurrent calls needing the same scopes now share one client-credentials request instead of racing several identical ones. - Granted scopes are verified when the server reports them. A narrower grant is accepted, since RFC 6749 section 3.3 allows it; a grant sharing nothing with the request fails immediately with both lists named. The shared MSW token mock returned a fixed `scope: "content"` for every request. It now echoes the requested scope, as a real authorization server does for an approved client -- otherwise the granted-scope check is untestable and scope selection bugs stay hidden. generate-operation-catalogs gains --source-ref to pin the docs revision, and warns when a build reads qf-api-docs main. The generated catalog is a published artifact, so reading it from a moving branch means a rebuild can change client behaviour with no change in this repository. Tests: 29 new (packages/api/test/content-scopes.test.ts) driving a real fetcher with a fake fetch and asserting the scopes that actually leave the SDK. Full suite 181 passed / 0 failed (was 152). Lint clean, tsup build succeeds, catalog --check up to date. Changeset added for the release notes and minimum-version statement. Note: `tsc --noEmit -p packages/api` reports 28 pre-existing strictness errors in existing tests and mocks; none is in a file this change touches, and packages/api has no `typecheck` script, so it is not part of the repo's checks. Refs: Content Scope Split plan work package T10, decision D6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
size-limit report 📦
|
Collaborator
Author
The one red check is a cross-repo ordering dependency, not a defect here
Verified locally: the same check passes against a docs tree that has the metadata. This is the T09 → T10 dependency in the plan's own graph — docs must land before the SDK. Merge #207 first, then re-run this check and it goes green with no change here. I deliberately did not work around it. The two available hacks both make things worse: pinning 🤖 Generated with Claude Code |
Two review findings, both real.
1. Granular mode broke every documented typed method.
resolveAppScopes() required scope metadata on an OperationDefinition. The generated
raw operations carry one, so those worked. The typed convenience facades do not:
client.content.v4.chapters.list() reaches QuranChapters.findAll(), which calls
fetcher.fetch("/content/api/v4/chapters") with a plain URL and no descriptor. So
granular mode threw on the whole public API while the raw API worked. My earlier
tests only exercised requestOperation(), which is exactly why they missed it.
Fixed at the root rather than facade by facade: src/lib/content-scope-lookup.ts
resolves an operation's scopes from service + method + path against the same
pinned catalog. One lookup repairs every facade, present and future.
The lookup is deterministic, not heuristic. A literal path always wins over a
templated one, mirroring how the gateway resolves exact keys before dynamic ones.
A path parameter matches exactly one segment, so /chapters/{id} cannot swallow
/chapters/{id}/info. Where several templates match, the most specific wins. A path
genuinely absent from the contract still returns nothing and still produces the
explicit error -- there is no fall back to broad `content`.
Tests: new test/content-scopes-public-api.test.ts instantiates createServerClient
and calls its real public methods -- chapters, verses, juzs, audio, translations,
tafsirs, languages, answers, hadith references -- asserting the exact scope each
one requests, in both modes. Also asserts all three documented entrypoints for the
same operation (top-level facade, content.v4 facade, raw operation) resolve
identically.
One existing test asserted that an untyped fetch throws in granular mode. That
premise was the bug, so it now asserts the contract lookup resolves it, and a new
test keeps the refusal behaviour using resources/changes, which is genuinely
unassigned.
2. The catalog source was not actually pinned.
Adding an optional --source-ref did not change what CI runs. CI invokes
check:operation-catalogs with no arguments, which read the mutable qf-api-docs main
branch -- so the check was both non-reproducible and failing.
scripts/openapi-source.json now records the approved revision, and generation,
--check and CI all resolve their default from that one committed file. The
generator warns when the pinned ref is not a full commit SHA, because a branch is
not a reproducible input.
The pin is read lazily and tolerantly: test/operation-catalog-generator.test.js
copies the generator to a temp directory and imports it standalone, where the pin
file is absent. A missing pin falls back to the branch and says so rather than
failing at import. CI runs from a full checkout, so CI is always pinned.
It currently points at the head of qf-api-docs feat/content-scope-split-v1, the
revision carrying x-qf-scopes, with a TODO to move it to the resulting commit on
main once quran/qf-api-docs#207 lands. The catalog was regenerated from that exact
revision, so `check` passes with no arguments.
Tests: 209 passed / 0 failed (was 181). Lint clean, tsup build succeeds,
check:operation-catalogs passes with CI's exact invocation.
Refs: review findings 2 and 3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nlyScopes The pinned qf-api-docs revision now carries contract sha256 ecfced4348e5, which records that `content.reflections.read` must not authorize a user-bound token. Regenerating against it produces byte-identical catalogs, which is the expected result: the new field is enforcement metadata for the gateway, and the catalogs carry authorization alternatives. Recorded in the pin's comment so the next person does not have to re-derive why nothing moved. This SDK cannot reach the restricted case in any event. Content scopes are resolved only on the `client_credentials` branch of `applyAuthHeaders`, where Hydra sets `sub` to the client id, so the gateway's check does not apply. The `user` branch sends the stored session's token and never chooses scopes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a72703074
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adadbab402
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
2 participants
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.
Adds an opt-in
contentScopeMode: "granular"so the SDK can request the specific scope each contentoperation needs, instead of one service-wide
contentscope.Context
Part of the Content Scope Split — Cross-Repository Implementation & Migration Plan.
The granular content scopes are not issued to any client yet, so no current integration is
affected. Contract:
content-scopes/v1, sha256ecfced4348e51237ecfe717303d2b180bcaa00a024ae078f2663516f018ba47bNothing changes unless you opt in
contentScopeModedefaults to"legacy", which reproduces the previous behaviour exactly —including the existing path-sniffing special case that mapped public QuranReflect reads to
post.readandcomment.read. Existing credentials and existing code are unaffected, and a testasserts the legacy token requests are unchanged.
Changing the default is a deliberate major-release decision (D6), not something this PR does.
Granular mode
A content call requests the single scope its operation declares. Scope metadata flows from the
contract into the OpenAPI documents (
x-qf-scopes) and from there into the generated operationcatalog, so the SDK does not re-derive policy from a path or a service name.
Quota fields are deliberately left out of the catalog: rate limiting is the gateway's business and
must never be an authorization input in a client.
Deliberate refusals rather than fallbacks
client.fetch()call carries no operation, so granular mode cannot know its scope. Itraises an actionable error naming the escape hatches instead of quietly requesting
content,which would defeat the mode and fail anyway for credentials never granted it.
invalid_scoperejection is reported with the requestedscopes and is never retried with a broader scope. Widening on failure would paper over a
misconfigured client and hand it more access than it holds.
searchandanalytics.events.writestay separate permissions and are never requested for acontent call, in either mode.
Token handling fixes found while doing this
service:scope, which reused one issuer's token against another. It nowcovers the token endpoint, client, audience and canonical scope set, and scope order no longer
produces duplicate cache entries.
racing several identical ones.
RFC 6749 §3.3 allows it; a grant sharing nothing with the request fails immediately with both
lists named.
Test mock made realistic
The shared MSW token mock returned a fixed
scope: "content"for every request. It now echoes therequested scope, as a real authorization server does for an approved client — otherwise the
granted-scope check is untestable and scope-selection bugs stay hidden.
Source pinning
generate-operation-catalogsgains--source-refto pin the docs revision, and warns when a buildreads
qf-api-docsmain. The generated catalog is a published artifact, so reading it from amoving branch means a rebuild can change client behaviour with no change in this repository.
Tests
29 new in
packages/api/test/content-scopes.test.ts, driving a real fetcher with a fake fetch andasserting the scopes that actually leave the SDK. Full suite 181 passed / 0 failed (was 152).
Lint clean,
tsupbuild succeeds, catalog--checkup to date. Changeset included for the releasenotes and the minimum-version statement.
Note
tsc --noEmit -p packages/apireports 28 pre-existing strictness errors in existing tests andmocks; none is in a file this change touches, and
packages/apihas notypecheckscript, so it isnot part of the repo's checks.
Changes since review
Granular mode breaks the typed methods (high priority) and operation-catalog generation not
consistently pinned (CI blocker) were both addressed earlier in this branch:
packages/api/src/lib/content-scope-lookup.tsdoes a deterministic method/path lookup againstthe pinned contract, and
resolveAppScopesfalls back to it, so the public typed facades resolvegranular scopes instead of falling back to broad
content. 27 tests drive it through the realcreateServerClientpublic methods rather than through the helper.scripts/openapi-source.jsonrecords the docs revision explicitly, and generation,--checkandCI all read that one file.
Since the review, the pin moved to the docs revision carrying contract
ecfced4348e5, which addsappCredentialsOnlyScopes. Regenerating against it produces byte-identical catalogs - the newfield is enforcement metadata for the gateway, and the catalogs carry authorization alternatives.
Recorded in the pin's comment so the next person does not have to re-derive why nothing moved.
This SDK cannot reach the restricted case in any event: content scopes are resolved only on the
client_credentialsbranch ofapplyAuthHeaders, where Hydra setssubto the client id, so thegateway's check does not apply. The
userbranch sends the stored session's token and neverchooses scopes.
🤖 Generated with Claude Code