docs: add public API architecture plan and ADRs - #1879
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a documentation set for the planned LFX Insights Public API, including a detailed project plan, an architecture review packet, and a set of ADRs intended to lock down key contract and infrastructure decisions for a standalone /api service.
Changes:
- Introduces a comprehensive public API project plan covering architecture, epics, rollout stages, and observability strategy.
- Adds an “architecture review” doc set (overview, decisions, domain context) to present the proposal for approval.
- Adds ADRs formalizing major decisions (framework, versioning contract, auth model, caching, docs stack, etc.).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/PUBLIC_API_PLAN.md | Full project plan for the standalone public API service (architecture, epics, rollout, metrics). |
| docs/CONTEXT.md | Canonical domain language + wire-format conventions for the public API. |
| docs/architecture-review/01-overview.md | Architecture review overview document for stakeholders/approvers. |
| docs/architecture-review/02-decisions.md | Summary of “ADR-bar” decisions with links to ADRs. |
| docs/architecture-review/03-context.md | Architecture-review version of the canonical domain context. |
| docs/adr/0001-fastify-over-nestjs.md | ADR selecting Fastify over NestJS/Express/Hono. |
| docs/adr/0002-api-at-repo-root.md | ADR placing the service at repo-root api/ (not under workers). |
| docs/adr/0003-tolerant-reader-versioning.md | ADR defining /v1-alpha → /v1 stability and additive-only contract. |
| docs/adr/0004-server-to-server-cors-deny.md | ADR for server-to-server only (CORS) posture in v1. |
| docs/adr/0005-tiers-control-rate-limits-only.md | ADR for tier impact limited to rate limits in v1. |
| docs/adr/0006-long-lived-api-keys.md | ADR for long-lived, manually-rotated API keys. |
| docs/adr/0007-collections-only-permission-check.md | ADR limiting permission checks to Collections endpoints only. |
| docs/adr/0008-typebox-code-first-openapi.md | ADR choosing TypeBox code-first schemas as OpenAPI source. |
| docs/adr/0009-api-key-required-for-all-requests.md | ADR requiring API key auth for every request (no anonymous access). |
| docs/adr/0010-billing-bundled-with-lfx-membership.md | ADR bundling API access with existing LFX membership. |
| docs/adr/0011-pagination-page-pagesize-zero-indexed.md | ADR standardizing zero-indexed page/pageSize pagination. |
| docs/adr/0012-url-port-strategy-hybrid.md | ADR describing hybrid URL port/rename strategy. |
| docs/adr/0013-origin-cache-only-private-cache-control.md | ADR specifying origin-only Redis caching + Cache-Control: private. |
| docs/adr/0014-camelcase-json-iso8601-dates.md | ADR standardizing camelCase JSON + ISO-8601 UTC timestamps. |
| docs/adr/0015-api-keys-stored-in-auth0.md | ADR storing/managing API keys in Auth0 via Management API. |
| docs/adr/0016-vitepress-scalar-api-docs.md | ADR choosing VitePress + Scalar for API docs in api/docs/. |
| docs/adr/0017-collections-queries-not-shared.md | ADR keeping Collections SQL read queries in /api (not shared). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
emsearcy
left a comment
There was a problem hiding this comment.
Left some comments throughout. A couple high-level notes:
Are you familiar with https://github.com/linuxfoundation/lfx-architecture-decisions/tree/main/decisions ? In particular I didn't see anything here about logging framework.
(Caveat, ADR-0003 requires sending both OTEL and DataDog-formatted trace and span IDs (both 64-bit unsigned ints, as dd.trace_id and dd.span_id) -- this is out of date; Datadog now handles proper OTEL trace/span IDs natively.)
Also, while I get that you may all be Javascript devs, and I think overall your choices are really good, we often times prefer Go when possible for backend services, because it tends to be easier to maintain long term (forward portability guarantees). But I get it if JS is deemed easier by y'all.
|
|
||
| Keys do not expire automatically. Multiple active keys per user are supported for zero-downtime rotation (mint new → switch → revoke old). Revocation is enforced by deleting the key from Auth0 — the next request using it fails JWKS verification instantly. No deny-list needed. | ||
|
|
||
| Refresh tokens and expiring tokens can be introduced in v2 — long-lived keys are a simplicity decision for v1. |
There was a problem hiding this comment.
This is in conflict with ADR 15. Auth0 cannot provide long-lived API keys. It can either provide client_credential grants (exchange a long-lived client ID and secret for a short-lived access token) or authorization_code + refresh_token grants (login to get a refresh token, exchange the refresh token for a short-lived access token).
So either we need to change ADR 15 to adopt Postgres storage of long-lived keys, or change this one and adopt OAuth2 patterns earlier than v2.
There was a problem hiding this comment.
@jonathimer imo this would also be a business decision as it will affect user experience:
Long-lived keys (change ADR 15, store in Postgres):
- User generates a key once in the dashboard, pastes it into their app/script, and it just works forever (or until they revoke it).
- No login flow, no token refresh logic on their side.
- Same key works across machines, CI, scripts — easy to share within a team (which is also the security downside).
- If the key leaks, it's valid until someone manually revokes it.
- Not tied up with OAuth2.
OAuth2 now (change this ADR):
- Two sub-flavors depending on the use case:
- Machine-to-machine (client credentials): user gets a client ID + secret, their code exchanges them for a short-lived token and refreshes it when it expires. Extra code on their side, but standard.
- User-facing (auth code + refresh): user goes through a login/consent screen in the browser, app stores a refresh token. Better for apps acting on behalf of a user, but means no headless "paste and go."
- Tokens expire, so leaks have a limited blast radius.
- More upfront integration work for the user, but it's a known pattern.
There was a problem hiding this comment.
You're right that this was internally inconsistent. We've adopted the OAuth2 refresh token + short-lived access token model rather than trying to force long-lived tokens through Auth0.
What changed:
- ADR-0006 is renamed and rewritten as 0006-refresh-and-short-lived-access-tokens.md. The "API key" a customer receives is now explicitly a refresh token issued by LFX Self-Serve at app.lfx.dev/settings.
- Customers exchange it at POST api.insights.linuxfoundation.org/v1/auth/token — a thin proxy Insights exposes to Self-Serve's token endpoint — to get a short-lived access token (~15 min). The Bearer value sent to the Insights API is always the short-lived access token. - ADR-0015 updated to align: keys are refresh tokens from LFX Self-Serve, not Auth0-managed credentials.
There was a problem hiding this comment.
The design no longer uses Auth0-issued long-lived tokens at all: PATs issued by LFX Self-Serve are exchanged for short-lived Auth0 JWTs via Custom Token Exchange
|
|
||
| ### Pagination: `page` + `pageSize`, zero-indexed — [docs/adr/0011](../adr/0011-pagination-page-pagesize-zero-indexed.md) | ||
|
|
||
| All paginated endpoints use `page` (zero-indexed) + `pageSize` query params, returning `{ data, page, pageSize, total }`. The existing Nuxt codebase already uses this convention as the dominant pattern — preserving it avoids off-by-one translation bugs during the port. A handful of Nuxt endpoints use `limit`/`offset` instead; those are normalized to `page`/`pageSize` at port time so the public API stays consistent. External developers used to 1-based pagination will need to start at `page=0` — this is called out prominently in the docs quickstart. |
There was a problem hiding this comment.
This is OK, but I would challenge that opaque pagination tokens/cursors are a better design choice when you're starting from scratch. Also I recommend that any pagination standard also define sorting mechanism (hard coded vs. user-selected, but it needs to be defined).
- The user cannot "cheat" and grab the first page to get a total count, then grab all remaining pages in parallel: requests are forced to be serialized
- Cursors can (and should!) be implemented to provide atomicity, which page offset numbers cannot: the cursor acts as a sort of session to ensure consistency while paging. offset based pagination can introduce repeats or missed data entries. That is: between fetching page N and page N+1, if an item is added which, based on your sort semantics, adds or removes data in pages 0-N, it shifts the results by 1 either way. A lost entry is caused when the item which would have been the first item of page N+1 falls back into the last item of page N, but we don't see it because we already fetched page N, and a duplicate is caused by an insertion causing the last item of page N (that we already fetched) to also be the first item of page N+1
There was a problem hiding this comment.
Agreed on both points. Two changes:
- Cursor-based pagination is now the standard — ADR-0011 is renamed and rewritten as 0011-pagination-cursor-based.md. We dropped the zero-indexed page/pageSize offset model. Your atomicity example (insert between page N and N+1 → first item of N+1 falls back to last of N, silently skipped) is incorporated verbatim into the ADR as reason Frontend init #1.
- Sorting is now defined: each paginated endpoint declares a closed allow-list of accepted sort values in its TypeBox schema. Wire format: ?sort=field_direction (e.g. name_asc, commits_desc) — same convention as the existing Nuxt layer. Every accepted value must be index-backed. Removing an allowed value or changing an endpoint's default sort is a breaking change under ADR-0003. Defined in the Sort order section of ADR-0011 and in the wire-format conventions in CONTEXT.md.
| _Avoid:_ account, customer, client | ||
|
|
||
| **Organization** (`org_id`) | ||
| The LFX organization a User belongs to, extracted from the JWT. Used as the shared bucket for rate-limit quotas — all API keys belonging to users in the same org draw from one pool. |
There was a problem hiding this comment.
FWIW, Auth0 JWTs do not carry organization information at present. Also need to define "belongs to". There are org-admin authorized individuals (key contacts for memberships, mostly). If the scope of "belongs to" is intended to capture all employees, note that we leave the realm of authorized identities and have moved over to, essentially, self-attestations (I can say I work for anyone). Alignment of employee identity (and ongoing validation thereof) by known employer domains is not presently in scope for LFX as far as I know.
There was a problem hiding this comment.
On the JWT org claim: the org claim is now explicitly attributed to the LFX Self-Serve access token, not an Auth0 JWT. The auth model no longer involves Auth0 at the Insights API layer — customers get a refresh token from app.lfx.dev/settings, exchange it at POST /v1/auth/token (an Insights-proxied endpoint to Self-Serve), and receive a short-lived access token that Self-Serve signs and that Insights JWKS-verifies.
On "belongs to": narrowed to "authorized Key Contact of an organization with an active LFX membership — not every employee or self-attested affiliate." The Key Contact check lives in Self-Serve (OpenFGA against v2_organization entities), but the precise moment depends on an open product question (ADR-0015 Q1):
- If a new Insights-scoped refresh token is issued: check happens at issuance — non-Key-Contacts can't obtain the token at all.
- If the existing PAT is reused: everyone holds a PAT already, so the check moves to POST /v1/auth/token exchange time — Self-Serve refuses to mint an Insights access token for non-Key-Contacts even if they possess the PAT.
Either way, the check is in Self-Serve, and the Insights API doesn't touch a membership system. Updated in CONTEXT.md, 03-context.md, ADR-0010, and the Relationships section.
One assumption we'd like your input on: the spec assumes the LFX Self-Serve access token carries org and tier claims that Insights can read at request time for rate limiting. If those claims can't be added, the enforcement model changes significantly. Can you confirm whether Self-Serve can include these?
There was a problem hiding this comment.
Updating this thread since the auth model changed with the PAT + Custom Token Exchange redesign (#2061):
On the JWT org claim: org/tier no longer comes from JWT claims. The Cloudflare Worker resolves them from the LFX Tier endpoint and passes them to the Insights API as trusted headers (ADR-0006 variant 4b). The Auth0-signed JWT carries only identity (iss/sub/aud); no org information is expected in it. This also supersedes the question earlier in this thread about Self-Serve adding org/tier claims to access tokens.
On "belongs to": narrowed to authorized Key Contact of an organization with an active LFX membership (ADR-0010, "Where membership is enforced") not every employee or self-attested affiliate. Self-Serve gates Insights-audience PAT issuance on Key Contact status, and the Worker's fail-closed Tier lookup re-checks entitlement at request time, so a lapsed membership stops working without relying on manual PAT cleanup.
| _Avoid:_ error body, error payload | ||
|
|
||
| **Request ID** | ||
| A ULID generated per-request, propagated as the `X-Request-Id` response header and attached to all log lines and OTel spans. Used by support for tracing a specific request across systems. |
There was a problem hiding this comment.
OTEL defines the standard for trace IDs and span IDs, AND how they are propagated. We should not invent our own. This is imperative for universal tracing.
There was a problem hiding this comment.
👍 What changed (ADR-0019, ADR-0018, CONTEXT.md):
- X-Request-Id response header dropped entirely.
- W3C traceparent is now the sole HTTP propagation channel — honoured inbound, injected outbound automatically by the OTel SDK. No custom header.
- The only customer-facing exposure of the trace ID is requestId inside the error envelope JSON — that's the value a customer quotes in a support ticket, and it lives in a schema we already own, not a new HTTP header.
| ┌─────────────────────┐ 1. create key ┌─────────────────────────────────────┐ | ||
| │ User (browser) │ ───────────────▶ │ LFX Insights frontend │ | ||
| │ │ │ /settings/api-keys │ | ||
| │ │ ◀─────────────── │ (membership check → Auth0 Mgmt API)│ |
There was a problem hiding this comment.
No where in the spec is it defined how the membership check is wired up. (e.g. if this is using OpenFGA relationships against v2_organization entities, as discussed in F2F?) ADR 10 references this but points to Public API plan, and I don't see anything in this file actually defining it.
Ideally our spec would be prescriptive about the implementation. Or at least a statement "this is out of scope" or explains the human contract needed to fulfill the behavior -- so that AI implementation doesn't go off the rails and try to build something "wrong".
There was a problem hiding this comment.
Added a "Where membership is enforced" section to ADR-0010 that's explicit about the implementation
There was a problem hiding this comment.
The membership check wiring is now spelled out in ADR-0010, "Where membership is enforced". Enforcement is split across two boundaries:
- PAT-issuance time (LFX Self-Serve): Self-Serve gates issuance of an Insights-audience PAT on Key Contact status.
- Request time (Insights API): the API verifies the JWT and reads org/tier from the Worker-set headers for rate limiting. It never re-queries OpenFGA or any membership system.
|
Hey @jonathimer can you review the main files of this architecture proposal to double check if it's aligned with the initial PRD spec? It would basically be all files outside of
Also left you a comment based on a comment from Eric to try to understand how we should define tiers and org mapping with the user. |
| ### Key management UI | ||
|
|
||
| API keys are created and managed by users inside LFX Insights (not a separate LFX platform). Key creation is gated on the user's Organization holding an active LFX membership. The UI ([E15](../PUBLIC_API_PLAN.md#epic-e15--api-key-management-ui-lfx-insights-frontend)) covers: membership check, create/list/revoke keys, one-time key display on creation, and closed-alpha access gating. This is a hard dependency for the closed-alpha launch. |
There was a problem hiding this comment.
@jonathimer we some input on how users should create or have access to their API Keys.
What we proposed in this document was: "users are able to get a long-lived token on Insights with their org entitlements". With this information we would be able to also have access to the tier.
- @emsearcy mentioned that this is not possible as of today. In order for Eric to be able to advise us on how to get the token and the required info, we would need to understand:
- If "belongs to an organization" implies all employees of an org
- OR only allowlist-authorized individuals (key contacts or similar manual curated list)
#1879 (comment)
From Eric:
FWIW, Auth0 JWTs do not carry organization information at present. Also need to define "belongs to". There are org-admin authorized individuals (key contacts for memberships, mostly). If the scope of "belongs to" is intended to capture all employees, note that we leave the realm of authorized identities and have moved over to, essentially, self-attestations (I can say I work for anyone). Alignment of employee identity (and ongoing validation thereof) by known employer domains is not presently in scope for LFX as far as I know.
|
@emsearcy, thanks for the review Answered the individual inline comments in their respective threads.
|
emsearcy
left a comment
There was a problem hiding this comment.
I'm still concerned with the ambiguity of the API keys. Self Service (which is an Angular UI) should not be handling token exchange. Refresh tokens are probably not the best option for this, either. We probably would be better off with our own signed tokens, or possibly some kind of token service for long-lived tokens. I'll chat with the SSO team in DevOps this coming week.
|
|
||
| The customer-facing credential is a **refresh token** issued by the LFX Self-Serve App at `app.lfx.dev/settings`. It does not expire automatically. Rotation is encouraged (documented best practice) but never enforced — multiple active refresh tokens per User are supported so rotation is zero-downtime: mint new → switch integrations → revoke old. | ||
|
|
||
| Customer code exchanges the refresh token for a short-lived **access token** (~15 min; exact lifetime confirmed at T-015) via `POST api.insights.linuxfoundation.org/v1/auth/token`. That endpoint is a thin proxy to LFX Self-Serve's `/token` endpoint (RFC 6749 §6 `grant_type=refresh_token`). The Insights API forwards the request and returns the response verbatim — it does not mint, validate, or store refresh tokens. |
There was a problem hiding this comment.
Self service doesn't have a /token endpoint?. Auth0 is our IdP, and it's the one that exchanges refresh tokens for access tokens.
Next, our refresh tokens typically are configured with auto-rotation, which means each time you turn in the refresh token, you get not only an access token, but also your next refresh token.
This is a protection against lost/stolen tokens: if somebody else uses your refresh token, and you attempt to exchange the same refresh token, both are automatically invalidated, forcing a new interactive login.
All of these flows are based on the expectation that refresh tokens are automatically managed in the application state of an application (like a mobile app or desktop app) that can both handle an interactive login and manage its own state.
There was a problem hiding this comment.
Refresh-token design was dropped entirely in #2061. ADR-0006 is now PAT + Auth0 Custom Token Exchange:
The user sends the opaque PAT, and the Cloudflare Worker exchanges it for a short-lived Auth0-signed JWT transparently.
|
|
||
| 1. **No natural expiry on a leaked token.** A long-lived JWT that escapes into logs, error reports, or a compromised system stays valid indefinitely. Revocation requires Insights to run a per-request introspection-with-cache check — extra infrastructure for a problem the refresh-token model solves natively (leaked access token expires in ~15 min; leaked refresh token can only mint new access tokens, which the legitimate owner can stop by revoking it). | ||
|
|
||
| 2. **Revocation is ambiguous.** "Revoking" a long-lived JWT that has been cryptographically signed means nothing to a verifier that only checks the signature — the token remains valid until the key is rotated. Rotating the JWKS key revokes all tokens at once, not just one user's. The refresh-token model avoids this entirely. |
There was a problem hiding this comment.
correct, though I will add, revocation of a JWT is a thing, but it's usually done by creating a denylist: a denied JWT (even an access token!) has its unique ID (jti) added to a revocation list. Since JWTs would be expected to expire, you only need to keep it on the revocation list for the token lifetime—this avoids unbounded revocation lists. Of course, that doesn't make sense with an "expiration-less" JWT.
really one would be more likely to simple use a database of API keys, like GitHub PATs, rather than JWTs, for non-expiring credentials.
There was a problem hiding this comment.
JWTs only appear as the short-lived (~10 min) exchange output: revoking the PAT in Self-Serve makes the next exchange fail, and any already-issued JWT simply ages out.
A jti denylist could still be added later if the ~10 min revocation window ever needs to be tightened, and it stays bounded precisely because the JWTs expire. But for v1 the expiry alone is the revocation mechanism.
|
|
||
| API keys are refresh tokens issued by the LFX Self-Serve App at `app.lfx.dev/settings`. The Insights API exposes a proxied `/v1/auth/token` endpoint (forwarding to Self-Serve's `/token`) so customers configure only one host. Customer code exchanges refresh tokens for short-lived access tokens via that proxy (per ADR-0006); the Insights API receives only access tokens on actual API requests. The Insights API is a verifier only: it fetches the LFX Self-Serve JWKS endpoint, verifies the access token's signature, and reads identity + authorization claims from the verified payload. Insights stores no keys, runs no key-management UI, and has no dependency on the Auth0 Management API. | ||
|
|
||
| > **Note:** this decision assumes LFX Self-Serve can support the required token model (Key Contact gating, `org`/`tier` claims, JWKS exposure, and the `/token` proxy endpoint). Coordination with the Self-Serve team is required at T-015 before implementation — the exact shape of the solution may change based on what Self-Serve can provide. |
There was a problem hiding this comment.
What does Key Contact gating mean in this context? If you are providing different tiers, for example: are you only expecting this to gate on Key Contacts? or also to provide a "tier" claim in the ticket?
There was a problem hiding this comment.
Both, through separate mechanisms: Key Contact status only gates whether an Insights-audience PAT can be issued (and is re-checked at request time. The Worker rejects with 403 if the Tier endpoint no longer returns an active org/tier for the caller)
While tier travels separately, not as a claim in the suggested variant 4b, but as an x-tier header the Worker resolves from the LFX Tier endpoint, used solely for rate-limit pool sizing per ADR-0005. Detailed in ADR-0015, "What the Insights API reads from each request" and the request flow in ADR-0006.
| | `iss` | LFX Self-Serve issuer URL — used to select the right JWKS and reject foreign tokens. | | ||
| | `sub` | User ID — used for revocation reference and as the `customer_id` span attribute in APM traces. | | ||
| | `org` | LFX Organization ID — drives the rate-limit pool key (all Key Contacts in the same org share a pool). **Assumption:** Self-Serve includes this in the access token. Exact claim name and feasibility confirmed at T-015. | | ||
| | `tier` | LFX membership tier (`silver` / `gold` / `platinum`) — drives rate-limit pool size and any future per-route tier gating. **Assumption:** Self-Serve includes this in the access token. Confirmed at T-015. | |
There was a problem hiding this comment.
This is a big assumption—this is not trivial. Self Service does not create access tokens—so this is more of a need for some kind of Auth0 extensibility (like CDP roles is today). Also, projects have different tier names/structures (like Premier/Silver for ASWF): how do you want to handle that for the product side? Or, is claim only showing the organization's LF membership tier, e.g. I'm a Gold at CNCF but Silver at LF, so I get "Silver"?
There was a problem hiding this comment.
In the new suggested variant 4b no access-token claims are needed: the Worker resolves the tier from the LFX Tier endpoint and passes it as a trusted header. The claims path (4a) remains a documented fallback, with the Auth0 extensibility constraints recorded as its main drawback (ADR-0006, variants). On naming: it's the organization's LF membership tier (silver/gold/platinum), not per-project tiers; highest tier wins for multi-org Key Contacts (ADR-0015).
| | Claim | Purpose | | ||
| |---|---| | ||
| | `iss` | LFX Self-Serve issuer URL — used to select the right JWKS and reject foreign tokens. | | ||
| | `sub` | User ID — used for revocation reference and as the `customer_id` span attribute in APM traces. | |
There was a problem hiding this comment.
Hi — assuming we end up with JWTs in the final model - you might instead consider the claim http://lfx.dev/claims/username which we often add to access tokens to carry an LFID (the one you see with an auth0| prefix is NOT reliably a LF username. It is unique (and immutable) per user, so you can use it for internal access if you need to, but it is a violation of our LFID integration guide to attempt to extract an LFID by stripping the "auth0|" prefix.
You might consider using user.id or enduser.id for the span attribute, as both of the these are semantic conventions (and are already indexed facets in Datadog!)
https://opentelemetry.io/docs/specs/semconv/registry/attributes/enduser/
https://opentelemetry.io/docs/specs/semconv/registry/attributes/user/
(I think "user" is more used in applications or webapp RUM, and "enduser" is more appropriate for APIs, but I could be mistaken)
There was a problem hiding this comment.
Adopted both suggestions:
- The span attribute is renamed
customer_id→enduser.ideverywhere (§6 catalog, T-023, and the ADRs) substays an opaque unique ID used for the Worker's tier lookup and the Collections ownership check (we never strip theauth0|prefix), andhttp://lfx.dev/claims/usernameis now documented as theenduser.idvalue (ADR-0015).
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
33daec6 to
c48d6bd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Suppressed comments (30)
docs/adr/0001-fastify-over-nestjs.md:1
- This ADR does not follow the mandatory repository format.
.claude/rules/adr-format.md:18-28requires an# ADR-0001:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections; lines 43-52 also require adding it todocs/adr/README.md. Add the missing decision metadata and sections before publishing it.
# Fastify over NestJS for the public API service
docs/adr/0002-api-at-repo-root.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0002:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are all required.
# Public API service lives at `/api` (repo root), not inside `workers/`
docs/adr/0003-tolerant-reader-versioning.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0003:heading, Date/Status/Deciders metadata, and ordered Context/Decision/Alternatives/Consequences sections are required.
# v1 contract: tolerant-reader / additive-only changes within a version
docs/adr/0004-server-to-server-cors-deny.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0004:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# v1 is server-to-server only; CORS denies all browser origins
docs/adr/0005-tiers-control-rate-limits-only.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0005:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Tiers control rate limits only in v1; no per-endpoint feature gating
docs/adr/0006-refresh-and-short-lived-access-tokens.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0006:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Refresh tokens are long-lived; access tokens are short-lived
docs/adr/0007-collections-only-permission-check.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0007:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Only Collections endpoints require a per-request permission check; groups 1–5 are public-data-only
docs/adr/0008-typebox-code-first-openapi.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0008:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# TypeBox for code-first OpenAPI schema generation
docs/adr/0009-api-key-required-for-all-requests.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0009:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Every request requires a valid API key — no anonymous access
docs/adr/0010-billing-bundled-with-lfx-membership.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0010:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# API access is bundled with LFX membership; no standalone billing in v1
docs/adr/0011-pagination-cursor-based.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0011:heading, Date/Status/Deciders metadata, and ordered Context/Decision/Alternatives/Consequences sections are required.
# Pagination is cursor-based with opaque base64url cursors
docs/adr/0012-url-port-strategy-hybrid.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0012:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# URL porting strategy: port-as-is by default, rename only when genuinely misleading
docs/adr/0013-origin-cache-only-private-cache-control.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0013:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Responses are cached at the origin (Redis) only; Cache-Control: private, max-age=0
docs/adr/0014-camelcase-json-iso8601-dates.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0014:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# All JSON keys are camelCase; dates are ISO-8601 UTC strings
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0015:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# API keys are issued by the LFX Self-Serve App
docs/adr/0016-vitepress-scalar-api-docs.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0016:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# API docs use VitePress + Scalar, served from `api/docs/`
docs/adr/0017-collections-queries-not-shared.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0017:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Collections Postgres queries are written fresh in `/api`, not shared with the frontend
docs/adr/0018-structured-json-logging.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0018:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# Structured JSON logging via pino; log levels follow LFX-0002
docs/adr/0019-opentelemetry-instrumentation.md:1
- This ADR omits the mandatory format defined in
.claude/rules/adr-format.md:18-28: the# ADR-0019:heading, Date/Status/Deciders metadata, and Context/Decision/Alternatives/Consequences sections are required.
# OpenTelemetry instrumentation; OTel trace ID is the request ID
docs/PUBLIC_API_PLAN.md:166
- This wording appears to put the Tinybird, Redis, and Postgres checks on both health endpoints. Dependency outages must fail readiness, not liveness; otherwise Kubernetes can restart every healthy API process during an upstream outage and amplify the incident.
- **T-006** Standard health endpoints: `/health/live`, `/health/ready` (TB ping, Redis ping, PG ping).
docs/PUBLIC_API_PLAN.md:274
T-089is not defined anywhere in this plan, so the closed-alpha allowlist dependency cannot be turned into a Jira task or tracked. Add the missing allowlist task (or point this reference to the intended existing task).
- **T-096** Closed-alpha gating signal: surface a "request access" state for users whose org is not on the closed-alpha allowlist ([T-089](#epic-e16--pre-launch)), so they understand why their key (if any) returns 403 against `/v1-alpha`.
docs/PUBLIC_API_PLAN.md:281
- Task numbering is inconsistent: this is
T-091, the verification section also labels the security review asT-091, while the open question andCONTEXT.mdcall this tier taskT-093. Assign unique task IDs and update all references so generated Jira work does not map two deliverables to one ID.
- **T-091** Tier-to-API-access mapping finalized with product (per §9 #2 reuses existing LFX tiers, but rate-limit numbers per tier need product sign-off).
docs/PUBLIC_API_PLAN.md:371
- This 5–60 second contract contradicts this plan's line 92,
CONTEXT.md:97, ADR-0013, and the existing Nuxt values infrontend/setup/caching.ts:3-4, all of which specify 24 hours / 1 hour. Leaving both values makes the implementation target ambiguous.
18. **Caching contract (v1):** origin-side Redis cache only (~5–60s TTL depending on endpoint). All responses set `Cache-Control: private, max-age=0` — customers do not cache, intermediaries do not cache. Lets us tune TTL without breaking customers. Public/CDN cache headers can be introduced later as a non-breaking improvement once we have real traffic data.
docs/adr/0009-api-key-required-for-all-requests.md:3
- “All endpoints” conflicts with the unauthenticated Kubernetes probes introduced at T-006 and with
/v1/auth/token, which cannot require the access token it is meant to mint. Define the authentication floor for public analytics routes and explicitly carve out infrastructure/bootstrap routes with their own controls.
All endpoints, including those that expose public project data (Endpoint Groups 1–4), require a valid API key. There is no unauthenticated access path. A missing or invalid key returns 401 immediately. We chose the auth-floor approach because: (1) rate limiting and abuse prevention require a stable identity to enforce per-org quotas; (2) attribution — knowing which orgs use which endpoints — is essential for prioritizing the roadmap and justifying infrastructure cost; (3) anonymous access complicates the future tier-gating mechanism. The cost is a higher onboarding barrier (users must create an API key before their first request). This can be revisited if adoption data shows the friction is significant.
docs/adr/0011-pagination-cursor-based.md:23
- This says opacity permits changing cursor encoding without a version bump, but the plan and both context documents explicitly classify encoding-semantic changes as breaking. Opacity prevents clients from depending on fields; it does not keep already-issued cursors valid after an incompatible server change.
The cursor is `base64url(JSON.stringify({ k: <last sort-key value>, id: <tiebreaker id> }))`. It is server-generated and server-opaque: clients must not parse, construct, or store cursors as structured data. They pass back the `nextCursor` value verbatim. Opacity means the server can change the internal encoding (add fields, switch format) without issuing a breaking change.
docs/adr/0013-origin-cache-only-private-cache-control.md:8
private, max-age=0does not mean clients do not cache: it permits private caches to store the response but requires revalidation before reuse. If the contract is truly “customers do not cache,” useno-storeand update the repeated header definitions; otherwise describe the actual revalidation behavior.
Clients and intermediary proxies/CDNs do not cache responses (`Cache-Control: private, max-age=0`). We chose origin-only caching over public HTTP caching because: (1) some endpoints are user-scoped (Collections) where a shared CDN cache would be a security error; (2) origin Redis gives us a single TTL knob tunable without a contract change. Public cache headers can be introduced later as a non-breaking improvement once per-endpoint analysis is done. Engineers must not add `public` or `s-maxage` headers without a deliberate review.
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:14
- The planned telemetry emits
api_key_id, but this token contract defines no per-credential identifier.subidentifies the user andkididentifies the issuer's signing key, so neither distinguishes multiple active keys for one user. Add a stable per-PAT/token identifier claim (for example an agreedjtior credential ID) or remove the per-key telemetry requirement.
| `sub` | User ID — used for revocation reference and as the `customer_id` span attribute in APM traces. |
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:17
kidis a JOSE protected-header parameter, not a JWT payload claim. Listing it as a claim can lead the verifier to trust a payload field instead of selecting the verification key from the signed token header.
| `kid` | Key ID — selects the right key in the JWKS response for signature verification. |
docs/adr/0003-tolerant-reader-versioning.md:37
- The
Sunsetheader does not use ISO-8601 syntax; it requires an HTTP-date. Implementing this text would emit a non-conforming header that clients may fail to parse.
2. **Add response headers** on the affected endpoint: `Deprecation: true` and `Sunset: <ISO-8601 date>` (the earliest date we will remove it), plus `Link: <migration-guide-url>; rel="deprecation"` pointing to the migration guide.
docs/adr/0019-opentelemetry-instrumentation.md:22
- With the Node SDK's default parent-based sampler, an inbound remote
traceparentmarked unsampled causes the child request span to remain unsampled. Because this ADR also honors caller trace context, “100% sampling” is not true unless the sampler is configured explicitly; state the exception or specify the sampler needed to enforce the requirement.
100% sampling in v1 — closed-alpha and silent-public traffic is low absolute volume and we want every trace for debugging. Per LFX-0003, higher-throughput services should drop the sample rate; revisit once we have RPS data on `/v1`.
…2061) Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Suppressed comments (43)
docs/adr/0011-pagination-cursor-based.md:27
- Encoding the dropped lookahead row skips that row on the next request: a keyset query normally asks for rows after the cursor, so the extra row is neither returned now nor later. Use the last retained row as
nextCursor; the extra row only proves another page exists.
Tinybird queries fetch `pageSize + 1` rows. If the result set has `pageSize + 1` rows, there is a next page: drop the last row from the response, encode its `(sort_key, id)` as the `nextCursor`. If the result set has `≤ pageSize` rows, set `nextCursor: null`. No second query needed.
docs/adr/0013-origin-cache-only-private-cache-control.md:3
- A cache keyed only by request parameters is unsafe for private Collections: two users can request the same slug/parameters, allowing a response cached for the owner to be returned to a non-owner. Include verified identity in user-scoped cache keys and require the ownership check before cache return.
All responses carry `Cache-Control: private, max-age=0`. The origin stores a Redis cache keyed by request params, mirroring the two-tier TTL model already in use by the Insights Nuxt API (`frontend/setup/caching.ts`):
docs/adr/0001-fastify-over-nestjs.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Fastify over NestJS for the public API service
docs/adr/0002-api-at-repo-root.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Public API service lives at `/api` (repo root), not inside `workers/`
docs/adr/0003-tolerant-reader-versioning.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# v1 contract: tolerant-reader / additive-only changes within a version
docs/adr/0004-server-to-server-cors-deny.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# v1 is server-to-server only; CORS denies all browser origins
docs/adr/0005-tiers-control-rate-limits-only.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Tiers control rate limits only in v1; no per-endpoint feature gating
docs/adr/0007-collections-only-permission-check.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Only Collections endpoints require a per-request permission check; groups 1–5 are public-data-only
docs/adr/0008-typebox-code-first-openapi.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# TypeBox for code-first OpenAPI schema generation
docs/adr/0009-api-key-required-for-all-requests.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Every request requires a valid API key — no anonymous access
docs/adr/0010-billing-bundled-with-lfx-membership.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# API access is bundled with LFX membership; no standalone billing in v1
docs/adr/0011-pagination-cursor-based.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Pagination is cursor-based with opaque base64url cursors
docs/adr/0012-url-port-strategy-hybrid.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# URL porting strategy: port-as-is by default, rename only when genuinely misleading
docs/adr/0013-origin-cache-only-private-cache-control.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Responses are cached at the origin (Redis) only; Cache-Control: private, max-age=0
docs/adr/0014-camelcase-json-iso8601-dates.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# All JSON keys are camelCase; dates are ISO-8601 UTC strings
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# API keys are issued by the LFX Self-Serve App
docs/adr/0016-vitepress-scalar-api-docs.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# API docs use VitePress + Scalar, served from `api/docs/`
docs/adr/0017-collections-queries-not-shared.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Collections Postgres queries are written fresh in `/api`, not shared with the frontend
docs/adr/0018-structured-json-logging.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# Structured JSON logging via pino; log levels follow LFX-0002
docs/adr/0019-opentelemetry-instrumentation.md:1
- This ADR does not follow the mandatory repository template.
.claude/rules/adr-format.md:16-28requires an# ADR-NNNNheading, Date/Status/Deciders metadata, and Context, Decision, Alternatives Considered, and structured Consequences sections; most are missing here. Please add the required decision metadata and sections before merging.
# OpenTelemetry instrumentation; OTel trace ID is the request ID
docs/PUBLIC_API_PLAN.md:20
- This goal contradicts decision 14 below, which explicitly says usage is not invoiced and billing remains bundled with membership. Describing observability as enabling tier billing leaves implementation and product scope ambiguous; frame it around tier enforcement/monitoring instead.
- Heavy observability (OTel → Datadog) so we can offer SLAs and bill by tier confidently.
docs/PUBLIC_API_PLAN.md:206
- The instrumentation task requires
api_key_id, but the authentication contract exposes onlyiss,sub,kid, andaudin the JWT plus org/tier headers. The API therefore cannot populate this attribute or distinguish multiple PATs owned by one user. Either transport a verified PAT identifier to the API and document it in ADR-0006/0015, or removeapi_key_idfrom the metrics design.
- **T-023** Integrate OpenTelemetry SDK (`@opentelemetry/sdk-node`): HTTP auto-instrumentation, Postgres auto-instrumentation, custom spans around Tinybird calls. W3C TraceContext propagator (default). Span attributes carry high-cardinality dimensions (`customer_id`, `api_key_id`, `bucket_id`, `pipe`, numeric `status_code`). Per ADR-0019.
docs/PUBLIC_API_PLAN.md:281
- This task points to T-089, but no T-089 task exists in the plan, so the closed-alpha allowlist has no defined deliverable. Add the missing allowlist task or update this dependency to the intended existing task.
- **T-096** Closed-alpha gating signal: surface a "request access" state for users whose org is not on the closed-alpha allowlist ([T-089](#epic-e16--pre-launch)), so they understand why their key (if any) returns 403 against `/v1-alpha`.
docs/PUBLIC_API_PLAN.md:348
- T-091 is defined above as tier-to-rate-limit mapping, not a security review. This link therefore marks the wrong task as satisfying a release gate; add a security-review task and reference its ID here.
- Security review ([T-091](#epic-e16--pre-launch)).
docs/PUBLIC_API_PLAN.md:378
- This 5–60 second response-cache contract conflicts with the 24-hour/1-hour TTLs defined earlier in this plan and in ADR-0013. Since these values drive implementation and stale-data behavior, keep one authoritative pair.
18. **Caching contract (v1):** origin-side Redis cache only (~5–60s TTL depending on endpoint). All responses set `Cache-Control: private, max-age=0` — customers do not cache, intermediaries do not cache. Lets us tune TTL without breaking customers. Public/CDN cache headers can be introduced later as a non-breaking improvement once we have real traffic data.
docs/PUBLIC_API_PLAN.md:389
- T-093 is not defined; the product sign-off task is T-091. The broken reference leaves this open decision disconnected from its actual deliverable.
1. Rate-limit numbers per LFX membership tier (Gold, Platinum, etc.) — TBD, pending product sign-off. Drives [T-093](#epic-e16--pre-launch).
docs/PUBLIC_API_PLAN.md:390
- The PR description and the other review documents say 4b is only suggested and Insights stewards the final call with DevOps input. Calling it “planned” and assigning the final call to DevOps reverses both points, so this open question no longer reflects the recorded review outcome.
2. **Variant 4a vs 4b — where does tier resolution live?** 4b (planned) has the Cloudflare Worker resolve org and tier from an LFX Tier endpoint and pass them as headers; 4a has the PAT service enrich them into the JWT. The PAT, exchange, and verification path are identical either way, so this can be settled without reworking the rest. Final call sits with DevOps. See [ADR-0006](adr/0006-pat-token-exchange-for-api-credentials.md).
docs/PUBLIC_API_PLAN.md:325
- The estimate multiplies by four tiers, while the canonical tier list contains only Silver, Gold, and Platinum. Unless a fourth tier is defined, this overstates the cardinality budget and conflicts with the documented model.
**Cardinality budget (initial):** roughly `~25 endpoints × 4 tiers × 3 status_class × 2 versions ≈ 600 timeseries per metric` × 9 metrics ≈ 5.4k custom timeseries. Well within reasonable cost.
docs/CONTEXT.md:34
- T-093 is not defined in the project plan; tier hierarchy and rate-limit sign-off are assigned to T-091. Update the canonical glossary so readers can follow the reference.
A named LFX membership level attached to an Organization that controls the rate-limit pool size. Known tiers in ascending order: Silver, Gold, Platinum (exact hierarchy and rate-limit numbers confirmed at T-093). In v1, tiers affect only rate limits; endpoint-level gating is reserved for future versions.
docs/adr/0011-pagination-cursor-based.md:23
- Opacity prevents clients from depending on the cursor structure, but it does not make an encoding change automatically non-breaking: outstanding cursors encoded by the old server will fail unless the new decoder remains backward-compatible. This also contradicts PUBLIC_API_PLAN decision 17, which classifies cursor-encoding changes as breaking.
The cursor is `base64url(JSON.stringify({ k: <last sort-key value>, id: <tiebreaker id> }))`. It is server-generated and server-opaque: clients must not parse, construct, or store cursors as structured data. They pass back the `nextCursor` value verbatim. Opacity means the server can change the internal encoding (add fields, switch format) without issuing a breaking change.
docs/adr/0009-api-key-required-for-all-requests.md:3
- This limits public-data endpoints to Groups 1–4, while ADR-0007 and the decision summary explicitly include Group 5 (Overviews) and reserve permission checks for Group 6. Aligning the scope avoids implying that Overviews has a different authentication model.
All endpoints, including those that expose public project data (Endpoint Groups 1–4), require a valid API key. There is no unauthenticated access path. A missing or invalid key returns 401 immediately. We chose the auth-floor approach because: (1) rate limiting and abuse prevention require a stable identity to enforce per-org quotas; (2) attribution — knowing which orgs use which endpoints — is essential for prioritizing the roadmap and justifying infrastructure cost; (3) anonymous access complicates the future tier-gating mechanism. The cost is a higher onboarding barrier (users must create an API key before their first request). This can be revisited if adoption data shows the friction is significant.
docs/adr/0006-pat-token-exchange-for-api-credentials.md:15
- The accepted Decision commits to variant 4b, but this same ADR says 4a and 4b remain under consideration, and the PR description identifies that choice as still open. Keep the accepted decision variant-neutral or mark the ADR proposed until the entitlement placement is decided.
We will issue long-lived Personal Access Tokens from the LFX Self-Serve App and exchange them for short-lived Auth0-signed JWTs using Auth0 Custom Token Exchange, performed by a Cloudflare Worker in front of the Insights API. The exchange runs on a cache miss, not on every request — the Worker caches the exchanged JWT and the caller's entitlements for ~10 min. Membership org and tier are resolved by the Worker from a purpose-built LFX Tier endpoint and passed to the Insights API as trusted headers (variant 4b); the Insights API verifies the JWT and never sees the PAT.
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:11
- This calls the 4b claim/header split final even though ADR-0006 and the PR description leave 4a versus 4b open. Phrase this table as the proposed 4b shape, or document both sources until the decision is made.
The Insights API reads these values from the Worker-supplied JWT and headers. The final claim-vs-header split follows ADR-0006 variant 4b and is confirmed with DevOps before implementation.
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:16
subidentifies the user, not the individual PAT, and Insights performs no revocation lookup. Calling it a revocation reference is especially misleading when multiple active PATs per user are supported; PAT revocation uses the PAT service's token identifier/hash.
| `sub` | JWT claim | User ID — used as the revocation reference and the `customer_id` span attribute in APM traces. |
docs/adr/0006-pat-token-exchange-for-api-credentials.md:101
- The PATs do not live in an Insights-owned store; this ADR and ADR-0015 assign storage to the shared LFX Self-Serve PAT service. “Our own store” contradicts the stated ownership boundary and could lead implementers to add Insights-side credential storage.
- No Auth0 200-token-per-user cap, because PATs live in our own store, and optional PAT expiry becomes possible.
docs/PUBLIC_API_PLAN.md:218
- T-030 consumes a single
api/openapi.json, while T-037 requires one OpenAPI artifact per version at/v1/openapi.json,/v2/openapi.json, etc. Define the versioned artifact(s) Scalar will load so adding v2 does not silently leave the reference on one ambiguous spec.
- **T-030** Embed Scalar on the reference page; wire it to ingest the generated OpenAPI spec (`api/openapi.json`) on every release. Serve the static VitePress build at `api.insights.linuxfoundation.org/docs` via Fastify's static file serving under `/docs`.
docs/adr/0011-pagination-cursor-based.md:9
- The insertion/deletion outcomes are reversed. Inserting before the current offset shifts an already-returned row onto the next page (duplicate), while deleting before it shifts an unseen row behind the next offset (miss). Correcting the example matters because it is the primary justification for this decision.
1. **Stability under mutations — no duplicates or missed entries.** Offset pagination produces corrupt iteration when the underlying set changes between page fetches. Concretely: if an item is inserted between fetching page N and page N+1, every subsequent row shifts one position forward. The item that was the first entry of page N+1 slides back into the last position of page N — which we already fetched — so it is silently skipped. The reverse happens on deletion: the item that was the last entry of page N drops into the first position of page N+1, so it appears in both pages. A cursor anchors to a row's sort-key value rather than its offset, so insertions and deletions between calls never affect which rows the caller sees next. This API serves analytics over data that grows continuously — commits, contributors, vulnerabilities — making offset instability a practical concern, not a theoretical one.
docs/adr/0003-tolerant-reader-versioning.md:37
- These header values are not standards-compliant. RFC 9745 defines
Deprecationas a Structured Field Date such as@1735689600, nottrue; RFC 8594 requiresSunsetto be an HTTP-date, not ISO-8601. Implementing the documented values will produce headers standards-aware clients cannot parse.
2. **Add response headers** on the affected endpoint: `Deprecation: true` and `Sunset: <ISO-8601 date>` (the earliest date we will remove it), plus `Link: <migration-guide-url>; rel="deprecation"` pointing to the migration guide.
docs/PUBLIC_API_PLAN.md:231
Deprecation: trueis invalid under RFC 9745; the field must contain a Structured Field Date (@<unix-seconds>). The Sunset value must be an RFC 8594 HTTP-date. Update the task so implementation does not ship non-standard wire values.
- **T-038** Deprecation/Sunset header support (`Deprecation: true`, `Sunset: <date>`, `Link: <docs>; rel="deprecation"`).
docs/architecture-review/02-decisions.md:73
- The summarized wire values are invalid: RFC 9745 uses a Structured Field Date for
Deprecation, and RFC 8594 uses an HTTP-date forSunset. Keepingtruehere will reintroduce the wrong contract even if the ADR is corrected.
When a field, parameter, or endpoint needs to eventually be removed, the deprecation process is: (1) annotate it as `DEPRECATED:` in the TypeBox schema so it surfaces in the OpenAPI spec; (2) add `Deprecation: true`, `Sunset: <date>`, and `Link: <migration-guide>; rel="deprecation"` response headers on every response from that route; (3) publish a changelog entry and notify known consumers; (4) honour the sunset window — removal before the `Sunset` date is a contract violation; (5) do the actual removal in `/v2`, not `/v1`. Full details in [docs/adr/0003](../adr/0003-tolerant-reader-versioning.md).
docs/CONTEXT.md:40
- The canonical list omits the Overviews and Leaderboard groups defined in the plan, and “released together as a unit” conflicts with the per-endpoint promotion model. This glossary is meant to drive task wording, so it must enumerate the actual seven groups and independent rollout behavior.
A logical cluster of related endpoints released together as a unit (Development, Contributors, Popularity, Security, Collections). Each group maps to a Jira epic.
docs/architecture-review/03-context.md:44
- This definition omits the Overviews and Leaderboard groups and says groups are released together, despite the next sentence and the plan specifying per-endpoint promotion. Align the canonical review context with the seven-group rollout.
A logical cluster of related endpoints released together (Development, Contributors, Popularity, Security, Collections). Each group maps to a Jira epic. Endpoints within a group are promoted through launch stages independently.
docs/architecture-review/03-context.md:79
- This says every Collection caller must prove access and describes only user-curated Collections, but ADR-0007 and CONTEXT.md say public Collections are open to any valid API key and curated/system Collections have no owner. Document the public/private distinction so implementers do not gate public Collections or reject system-owned ones.
A user-curated named group of projects, stored in Postgres. The only Endpoint Group (Group 6) that requires a per-request permission check — callers must prove they have access to the specific Collection they are querying.
…ix consistency issues Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (42)
docs/adr/0011-pagination-cursor-based.md:27
- The lookahead row must not be used as the cursor. If the next query applies
WHERE key < cursor, encoding the dropped(pageSize + 1)row excludes that row from the next page, so one record is skipped at every boundary. Drop the lookahead row but encode the last row actually returned to the client.
Tinybird queries fetch `pageSize + 1` rows. If the result set has `pageSize + 1` rows, there is a next page: drop the last row from the response, encode its `(sort_key, id)` as the `nextCursor`. If the result set has `≤ pageSize` rows, set `nextCursor: null`. No second query needed.
docs/architecture-review/03-context.md:117
- The “one org per user” relationship contradicts ADR-0006's accepted multi-org selection flow and the overview's own multi-org decision. Update this diagram so it does not erase the highest-tier/tie-break behavior implementers must support.
User ──is Key Contact of──▶ Organization (one org per user, v1)
docs/CONTEXT.md:82
- This relationship contradicts the accepted multi-org resolution in ADR-0006 and
PUBLIC_API_PLAN.md:367, where a User may be Key Contact for several organizations and one org is selected by tier/tie-break rules. Keeping “one Organization” in the canonical context will lead downstream tasks to omit the selection logic.
- A **User** is an authorized **Key Contact** of one **Organization** (v1); the **Organization** owns the **Rate-limit Pool**
docs/adr/0001-fastify-over-nestjs.md:1
- Adding the first ADRs requires updating the index table in
docs/adr/README.md(.claude/rules/adr-format.md:43-52), but it still contains only_none yet_. Add entries for ADR-0001 through ADR-0019 so contributors can discover these decisions and their statuses.
# Fastify over NestJS for the public API service
docs/adr/0013-origin-cache-only-private-cache-control.md:3
- Keying the origin response cache only by request parameters is unsafe for user-scoped Collections. Two users can request the same private collection slug with identical params; unless authorization runs before cache lookup and the key includes the response's authorization scope, the second user can receive the first user's cached private response.
All responses carry `Cache-Control: private, max-age=0`. The origin stores a Redis cache keyed by request params, mirroring the two-tier TTL model already in use by the Insights Nuxt API (`frontend/setup/caching.ts`):
docs/adr/0011-pagination-cursor-based.md:23
- This conflicts with the v1 contract, which explicitly classifies changing cursor encoding semantics as breaking (
PUBLIC_API_PLAN.md:377andCONTEXT.md:44). Opaqueness prevents clients from depending on the representation, but existing cursors still fail unless the server continues decoding the old format during a transition.
The cursor is `base64url(JSON.stringify({ k: <last sort-key value>, id: <tiebreaker id> }))`. It is server-generated and server-opaque: clients must not parse, construct, or store cursors as structured data. They pass back the `nextCursor` value verbatim. Opacity means the server can change the internal encoding (add fields, switch format) without issuing a breaking change.
docs/CONTEXT.md:34
- The plan defines the tier hierarchy/rate-limit sign-off as
T-091; there is noT-093. Point the canonical glossary at the existing task so implementers can trace this requirement.
A named LFX membership level attached to an Organization that controls the rate-limit pool size. Known tiers in ascending order: Silver, Gold, Platinum (exact hierarchy and rate-limit numbers confirmed at T-093). In v1, tiers affect only rate limits; endpoint-level gating is reserved for future versions.
docs/PUBLIC_API_PLAN.md:348
T-091is the tier-to-rate-limit mapping task, not a security review. The promotion criteria require security sign-off, but E16 currently has no task that delivers it; add a dedicated security-review task and link that task here.
- Security review ([T-091](#epic-e16--pre-launch)).
docs/PUBLIC_API_PLAN.md:389
T-093is undefined; the tier mapping and product sign-off are already assigned toT-091in E16. This broken reference also causes the glossary to point to a nonexistent task.
1. Rate-limit numbers per LFX membership tier (Gold, Platinum, etc.) — TBD, pending product sign-off. Drives [T-093](#epic-e16--pre-launch).
docs/PUBLIC_API_PLAN.md:281
T-089is not defined anywhere in this plan, so this dependency cannot be turned into a Jira task and the link only jumps to the E16 heading. Add the missing closed-alpha allowlist task or reference the task that owns it.
- **T-096** Closed-alpha gating signal: surface a "request access" state for users whose org is not on the closed-alpha allowlist ([T-089](#epic-e16--pre-launch)), so they understand why their key (if any) returns 403 against `/v1-alpha`.
docs/PUBLIC_API_PLAN.md:206
api_key_idcannot currently be populated by the API: ADR-0015's verified JWT/header contract contains no PAT ID, and ADR-0006 says Insights never receives the PAT. Define a non-secret stable key identifier as a signed claim or trusted Worker header (including its verification/forwarding contract), or remove this dimension from API telemetry.
- **T-023** Integrate OpenTelemetry SDK (`@opentelemetry/sdk-node`): HTTP auto-instrumentation, Postgres auto-instrumentation, custom spans around Tinybird calls. W3C TraceContext propagator (default). Span attributes carry high-cardinality dimensions (`enduser.id`, `api_key_id`, `bucket_id`, `pipe`, numeric `status_code`). Per ADR-0019.
docs/PUBLIC_API_PLAN.md:321
- A revoked PAT fails during the Worker/Auth0 exchange and, by design, never reaches the Insights API, so the API cannot emit an auth-failure metric with reason
revoked. Either instrument this metric in the Worker/exchange path or removerevokedfrom the API-side catalog.
| `api.auth.failures` | Counter | `reason` (invalid_jwt / expired / revoked / missing) | `api_key_id` (when known) | Auth bypass attempts |
docs/PUBLIC_API_PLAN.md:378
- This TTL contradicts this plan's architecture section (
PUBLIC_API_PLAN.md:98), ADR-0013, and the canonical context, all of which commit to 24h for stable data and 1h for time-series data. Leaving both values as decisions makes the cache implementation requirement ambiguous.
18. **Caching contract (v1):** origin-side Redis cache only (~5–60s TTL depending on endpoint). All responses set `Cache-Control: private, max-age=0` — customers do not cache, intermediaries do not cache. Lets us tune TTL without breaking customers. Public/CDN cache headers can be introduced later as a non-breaking improvement once we have real traffic data.
docs/adr/0003-tolerant-reader-versioning.md:37
- RFC 8594 requires
Sunsetto use an HTTP-date (IMF-fixdate), not ISO-8601. Implementing this text literally would emit a malformed standardized header; use a value such asSun, 30 Jun 2027 23:59:59 GMT.
2. **Add response headers** on the affected endpoint: `Deprecation: true` and `Sunset: <ISO-8601 date>` (the earliest date we will remove it), plus `Link: <migration-guide-url>; rel="deprecation"` pointing to the migration guide.
docs/adr/0009-api-key-required-for-all-requests.md:3
- The blanket auth rule also covers
/health/*,/docs, and OpenAPI artifacts defined elsewhere in this plan. Browser navigation to/docscannot attach a PAT, and probe endpoints need a separately defined access policy. Scope this decision to versioned data endpoints and explicitly handle non-data routes.
All endpoints, including those that expose public project data (Endpoint Groups 1–4), require a valid API key. There is no unauthenticated access path. A missing or invalid key returns 401 immediately. We chose the auth-floor approach because: (1) rate limiting and abuse prevention require a stable identity to enforce per-org quotas; (2) attribution — knowing which orgs use which endpoints — is essential for prioritizing the roadmap and justifying infrastructure cost; (3) anonymous access complicates the future tier-gating mechanism. The cost is a higher onboarding barrier (users must create an API key before their first request). This can be revisited if adoption data shows the friction is significant.
docs/adr/0019-opentelemetry-instrumentation.md:13
- An inbound
traceparentmay legitimately be reused across several API calls in the same distributed trace, so its trace ID identifies a trace, not one request. Multiple failures can therefore expose the samerequestId, making a support lookup ambiguous. Use a per-server-span identifier (or a separately generated request ID) for the error envelope while retaining the trace ID for correlation, or rename the field/contract totraceId.
Auto-propagation makes a separate ULID-based request ID redundant: every request already has a 128-bit OTel trace ID generated at the inbound HTTP span (or honoured from an inbound `traceparent`). We use that trace ID as our request identifier:
- Error envelope `requestId` field = OTel trace ID (32-char lowercase hex). This is the customer-facing ID for support tickets — it correlates directly to logs and APM traces in Datadog without translation.
docs/adr/0001-fastify-over-nestjs.md:1
- The repository ADR rule requires every
docs/adr/[0-9]*.mdfile to use the# ADR-NNNN:heading and include Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences sections in that order (.claude/rules/adr-format.md:18-28). This new ADR omits that required structure, so its status and decision provenance are not recorded.
This issue also appears on line 1 of the same file.
# Fastify over NestJS for the public API service
docs/adr/0002-api-at-repo-root.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0002:and the file must include Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order required by.claude/rules/adr-format.md:18-28. Without these fields, the decision has no recorded status or provenance.
# Public API service lives at `/api` (repo root), not inside `workers/`
docs/adr/0003-tolerant-reader-versioning.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0003:and the file must include Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order required by.claude/rules/adr-format.md:18-28. The existing topical sections do not supply the missing metadata or alternatives analysis.
# v1 contract: tolerant-reader / additive-only changes within a version
docs/adr/0004-server-to-server-cors-deny.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0004:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# v1 is server-to-server only; CORS denies all browser origins
docs/adr/0005-tiers-control-rate-limits-only.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0005:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# Tiers control rate limits only in v1; no per-endpoint feature gating
docs/adr/0007-collections-only-permission-check.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0007:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# Only Collections endpoints require a per-request permission check; groups 1–5 are public-data-only
docs/adr/0008-typebox-code-first-openapi.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0008:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# TypeBox for code-first OpenAPI schema generation
docs/adr/0009-api-key-required-for-all-requests.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0009:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# Every request requires a valid API key — no anonymous access
docs/adr/0010-billing-bundled-with-lfx-membership.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0010:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# API access is bundled with LFX membership; no standalone billing in v1
docs/adr/0011-pagination-cursor-based.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0011:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# Pagination is cursor-based with opaque base64url cursors
docs/adr/0012-url-port-strategy-hybrid.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0012:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# URL porting strategy: port-as-is by default, rename only when genuinely misleading
docs/adr/0013-origin-cache-only-private-cache-control.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0013:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# Responses are cached at the origin (Redis) only; Cache-Control: private, max-age=0
docs/adr/0014-camelcase-json-iso8601-dates.md:1
- This ADR omits the mandatory repository ADR structure: use an
# ADR-0014:heading and add Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences in the order specified by.claude/rules/adr-format.md:18-28.
# All JSON keys are camelCase; dates are ISO-8601 UTC strings
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0015:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# API keys are issued by the LFX Self-Serve App
docs/adr/0016-vitepress-scalar-api-docs.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0016:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# API docs use VitePress + Scalar, served from `api/docs/`
docs/adr/0017-collections-queries-not-shared.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0017:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# Collections Postgres queries are written fresh in `/api`, not shared with the frontend
docs/adr/0018-structured-json-logging.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0018:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# Structured JSON logging via pino; log levels follow LFX-0002
docs/adr/0019-opentelemetry-instrumentation.md:1
- This ADR does not follow the mandatory repository ADR template: the H1 must start
ADR-0019:and Date, Status, Deciders, Context, Decision, Alternatives Considered, and structured Consequences must appear in the order required by.claude/rules/adr-format.md:18-28.
# OpenTelemetry instrumentation; OTel trace ID is the request ID
docs/adr/0013-origin-cache-only-private-cache-control.md:8
private, max-age=0does not mean clients cannot cache: private caches may store the response, but it is immediately stale and must be revalidated before reuse. If storage is forbidden, useno-store; otherwise document the actual contract as “no shared caching and no reuse without revalidation.”
Clients and intermediary proxies/CDNs do not cache responses (`Cache-Control: private, max-age=0`). We chose origin-only caching over public HTTP caching because: (1) some endpoints are user-scoped (Collections) where a shared CDN cache would be a security error; (2) origin Redis gives us a single TTL knob tunable without a contract change. Public cache headers can be introduced later as a non-breaking improvement once per-endpoint analysis is done. Engineers must not add `public` or `s-maxage` headers without a deliberate review.
docs/PUBLIC_API_PLAN.md:399
- ADR-0016 is the VitePress/Scalar documentation decision and contains no Tinybird failure policy. This reference points implementers to the wrong contract; either add the missing Tinybird error-handling ADR with its actual number or remove the ADR citation.
- Tinybird errors: serve stale Redis cache, 503 if no cache. (ADR-0016)
docs/adr/0011-pagination-cursor-based.md:9
- Cursor pagination does not guarantee “no duplicates or missed entries” when the selected sort key itself changes. Allowed analytics sorts such as
commits_desccan move records across the cursor between requests, producing the same instability unless queries use a snapshot/as-of boundary or an immutable ordering key. Document that limitation or add snapshot semantics.
1. **Stability under mutations — no duplicates or missed entries.** Offset pagination produces corrupt iteration when the underlying set changes between page fetches. Concretely: if an item is inserted between fetching page N and page N+1, every subsequent row shifts one position forward. The item that was the first entry of page N+1 slides back into the last position of page N — which we already fetched — so it is silently skipped. The reverse happens on deletion: the item that was the last entry of page N drops into the first position of page N+1, so it appears in both pages. A cursor anchors to a row's sort-key value rather than its offset, so insertions and deletions between calls never affect which rows the caller sees next. This API serves analytics over data that grows continuously — commits, contributors, vulnerabilities — making offset instability a practical concern, not a theoretical one.
docs/adr/0011-pagination-cursor-based.md:46
- The cursor contains only
{ k, id }, so the server cannot reliably detect that a cursor created forname_ascis being reused withcommits_desc; it may instead pass a value of the wrong type into the query or return an incorrect page. Bind the selected sort identifier into the cursor and reject mismatches with a 400 response rather than relying only on documentation.
The cursor still encodes `(sort_key_value, id)`. The `sort_key_value` is whichever field the caller selected. The cursor is opaque — callers must not mix cursors across `sort` values; this is documented in the OpenAPI parameter description for each endpoint.
docs/PUBLIC_API_PLAN.md:372
- This blanket wording conflicts with the same plan's
/health/*,/docs, and OpenAPI routes. Scope the auth floor to versioned API data requests and leave those non-data routes to explicit probe/docs access policies, matching the corrected ADR contract.
12. **Authentication floor:** every request requires a valid API key. No anonymous access path. 401 on missing/invalid key, full stop.
docs/architecture-review/02-decisions.md:37
- “All endpoints” also includes the Fastify-served
/docs, OpenAPI artifacts, and/health/*routes described in this review. Those routes cannot all follow the customer PAT flow (for example, ordinary browser navigation to/docshas no Authorization header). Scope this summary to versioned data endpoints and define separate policies for non-data routes.
### Every request requires a valid API key — [docs/adr/0009](../adr/0009-api-key-required-for-all-requests.md)
All endpoints, including those serving public project data, require a valid API key. There is no unauthenticated path. A missing or invalid key returns 401 immediately. This is intentional: rate limiting requires a stable identity, and attribution data is essential for roadmap prioritization.
docs/PUBLIC_API_PLAN.md:378
private, max-age=0permits storage in private client caches; it only makes the stored response immediately stale and requires revalidation. Replace “customers do not cache” with the actual semantics, or useno-storeif client storage must be prohibited.
18. **Caching contract (v1):** origin-side Redis cache only (~5–60s TTL depending on endpoint). All responses set `Cache-Control: private, max-age=0` — customers do not cache, intermediaries do not cache. Lets us tune TTL without breaking customers. Public/CDN cache headers can be introduced later as a non-breaking improvement once we have real traffic data.
docs/architecture-review/02-decisions.md:103
- The stated client behavior is not what
Cache-Control: private, max-age=0enforces: private caches may store the response and must revalidate it before reuse. Say “no shared caching/no reuse without revalidation,” or chooseno-storeif clients must not retain responses.
All responses carry `Cache-Control: private, max-age=0`. A Redis cache with two TTL tiers lives at the origin: 24h for stable data (project lists, leaderboards, categories), 1h for time-series analytics. Clients and CDNs do not cache. This keeps a single TTL knob we can tune without a contract change, and avoids accidental public caching of org-scoped Collection responses.
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
|
Hi @emsearcy, since your last pass, the auth design has been reworked based on your feedback: long-lived keys are replaced with PATs issued by LFX Self-Serve, exchanged at the edge by a CF Worker for short-lived Auth0-signed JWTs via Custom Token Exchange (merged in #2061). I've also addressed the remaining points on pagination (cursor-based with defined sorting), OTel-standard trace propagation, membership check wiring, and org/tier resolution, and replied to each open thread with links to the relevant ADR sections. When you have a chance, could you take another look and resolve the threads that are now addressed? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (51)
docs/PUBLIC_API_PLAN.md:395
- This reverses both decisions recorded elsewhere: ADR-0006 calls 4b “suggested,” not planned, and says Insights stewards the 4a/4b call with DevOps input (also repeated in T-015 and the architecture overview). Keeping “planned” and assigning the final call to DevOps leaves ownership and status ambiguous.
2. **Variant 4a vs 4b: where does tier resolution live?** 4b (planned) has the Cloudflare Worker resolve org and tier from an LFX Tier endpoint and pass them as headers; 4a has the PAT service enrich them into the JWT. The PAT, exchange, and verification path are identical either way, so this can be settled without reworking the rest. Final call sits with DevOps. See [ADR-0006](adr/0006-pat-token-exchange-for-api-credentials.md).
docs/PUBLIC_API_PLAN.md:404
- ADR-0016 is the VitePress/Scalar decision, not a Tinybird failure policy. This line also says expired (“stale”) data is served, whereas CONTEXT.md:101 says expired entries yield 503. Link the actual decision and choose one cache-expiry behavior before implementation.
- Tinybird errors: serve stale Redis cache, 503 if no cache. (ADR-0016)
docs/PUBLIC_API_PLAN.md:286
- All
#epic-eN--...links in this file use an extra hyphen after the epic number. The generated anchors remove the colon and use one hyphen (for example,#epic-e16-pre-launch), so these links do not navigate to their target sections. Update every epic anchor in this file.
- **T-096** Closed-alpha gating signal: surface a "request access" state for users whose org is not on the closed-alpha allowlist ([T-089](#epic-e16--pre-launch)), so they understand why their key (if any) returns 403 against `/v1-alpha`.
docs/adr/0001-fastify-over-nestjs.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Fastify over NestJS for the public API service
docs/adr/0002-api-at-repo-root.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Public API service lives at `/api` (repo root), not inside `workers/`
docs/adr/0003-tolerant-reader-versioning.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# v1 contract: tolerant-reader / additive-only changes within a version
docs/adr/0004-server-to-server-cors-deny.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# v1 is server-to-server only; CORS denies all browser origins
docs/adr/0005-tiers-control-rate-limits-only.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Tiers control rate limits only in v1; no per-endpoint feature gating
docs/adr/0007-collections-only-permission-check.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Only Collections endpoints require a per-request permission check; groups 1–5 are public-data-only
docs/adr/0008-typebox-code-first-openapi.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# TypeBox for code-first OpenAPI schema generation
docs/adr/0009-api-key-required-for-all-requests.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Every request requires a valid API key; no anonymous access
docs/adr/0010-billing-bundled-with-lfx-membership.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# API access is bundled with LFX membership; no standalone billing in v1
docs/adr/0011-pagination-cursor-based.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Pagination is cursor-based with opaque base64url cursors
docs/adr/0012-url-port-strategy-hybrid.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# URL porting strategy: port-as-is by default, rename only when genuinely misleading
docs/adr/0013-origin-cache-only-private-cache-control.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Responses are cached at the origin (Redis) only; `Cache-Control: private, max-age=0`
docs/adr/0014-camelcase-json-iso8601-dates.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# All JSON keys are camelCase; dates are ISO-8601 UTC strings
docs/adr/0015-api-keys-issued-by-lfx-self-serve.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# API keys are issued by the LFX Self-Serve App
docs/adr/0016-vitepress-scalar-api-docs.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# API docs use VitePress + Scalar, served from `api/docs/`
docs/adr/0017-collections-queries-not-shared.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Collections Postgres queries are written fresh in `/api`, not shared with the frontend
docs/adr/0018-structured-json-logging.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# Structured JSON logging via pino; log levels follow LFX-0002
docs/adr/0019-opentelemetry-instrumentation.md:1
- This ADR omits the mandatory ADR heading/metadata and required sections defined in
.claude/rules/adr-format.md:16-31(# ADR-NNNN, Date, Status, Deciders, Context, Decision, Alternatives Considered, and Consequences). Please convert it to the repository ADR template so its approval state and trade-offs are recorded.
# OpenTelemetry instrumentation; OTel trace ID is the request ID
docs/PUBLIC_API_PLAN.md:21
- The rollout summary omits Overviews and Leaderboard and places Collections immediately after Security, while §5 defines a seven-group order with Overviews before Collections. Keep the top-level roadmap consistent with the detailed rollout.
This issue also appears on line 404 of the same file.
- Phased rollout: **Development → Contributors → Popularity → Security & Best Practices → Collections** (more later).
docs/PUBLIC_API_PLAN.md:117
- This comparison is factually stale: Express 5 was released as stable in September 2024, and Express 5 also improved async error propagation. Reassess the Express trade-offs using the released version rather than citing it as “imminent.”
| **Express** | Ubiquitous, every dev knows it, every middleware exists, simplest to debug, lowest learning curve. | No built-in validation or OpenAPI; ~half the throughput of Fastify; async error handling clumsy without wrappers; no opinionated structure: every team builds it differently; Express 5 has been "imminent" for years; least modern of the four. |
docs/PUBLIC_API_PLAN.md:157
- The cost claim conflates querying retained spans with generating metrics from spans. Datadog bills generated span-based metrics as custom metrics, and grouping them by
enduser.id/api_key_idcan cause the same cardinality growth this design is trying to avoid. Distinguish Trace Explorer/indexed-span drilldowns from generated timeseries or monitors, then include indexed-span/custom-metric costs in the budget before relying on per-customer dashboards.
| **APM trace metrics** (derived from span attributes) | Slicing by span attributes is not billed as custom metrics; can slice by `enduser.id` / `api_key_id` without cost spike; flame-graph + latency-breakdown per request; ingestion cost is per-span, not per-tag. | APM has its own ingestion cost; span sampling can drop rare events at scale; alerting ergonomics are slightly different; counters for rate-limit rejections still want every event. |
docs/PUBLIC_API_PLAN.md:223
- This task defines a single
api/openapi.json, but T-037 defines one artifact per version served at/v1/openapi.json. The build artifact and URL contract need one version-aware naming scheme; otherwise Scalar may ingest a different spec than the API exposes.
- **T-030** Embed Scalar on the reference page; wire it to ingest the generated OpenAPI spec (`api/openapi.json`) on every release. Serve the static VitePress build at `api.insights.linuxfoundation.org/docs` via Fastify's static file serving under `/docs`.
docs/PUBLIC_API_PLAN.md:286
T-089is not defined anywhere in the plan or repository, so this dependency cannot be turned into a Jira task. Add the missing allowlist task or point this requirement to its actual task ID.
This issue also appears on line 286 of the same file.
- **T-096** Closed-alpha gating signal: surface a "request access" state for users whose org is not on the closed-alpha allowlist ([T-089](#epic-e16--pre-launch)), so they understand why their key (if any) returns 403 against `/v1-alpha`.
docs/PUBLIC_API_PLAN.md:344
- The workspace currently includes submodule, worker, frontend, and services patterns (
pnpm-workspace.yaml:1-8), not justfrontendandworkers/*. Remove the inaccurate inventory while retaining the required additions.
- `pnpm-workspace.yaml`: currently lists `frontend`, `workers/*`. Add `api` and `libs/*` entries when bootstrapping ([T-001](#epic-e1--foundation--framework)).
docs/PUBLIC_API_PLAN.md:353
- T-091 is defined above as tier/rate-limit mapping, not a security review. As written, the promotion criteria require a security gate for which no task exists; add a dedicated security-review task and reference its ID here.
- Security review ([T-091](#epic-e16--pre-launch)).
docs/PUBLIC_API_PLAN.md:375
- This grouping implies Overviews may require the Collections permission check, contradicting ADR-0007 and
architecture-review/02-decisions.md:55-57, which make Groups 1–5 public-data-only and restrict ownership checks to Group 6. Separate Group 5 from Collections so the authorization boundary is unambiguous.
- **Phases 1–4 (Development, Contributors, Popularity, Security & Best Practices):** public OSS data, **no per-project permission check**. Tier check only.
- **Phase 5 (Overviews) + Phase 6 (Collections):** Collections add a tier check + permission check (private collections gated by the owner-only `collections.ssoUserId == sub` check per [ADR-0007](adr/0007-collections-only-permission-check.md); public collections open to all valid keys). Permission source: **Postgres lookup with Redis cache** (~60s TTL).
docs/PUBLIC_API_PLAN.md:383
- The
~5–60scache contract contradicts this plan’s §2, CONTEXT.md, ADR-0013, and the decisions summary, all of which specify 24h for stable data and 1h for time-series data. A 60-second implementation would miss the documented caching strategy and capacity assumptions.
18. **Caching contract (v1):** origin-side Redis cache only (~5–60s TTL depending on endpoint). All responses set `Cache-Control: private, max-age=0`. Customers do not cache, intermediaries do not cache. Lets us tune TTL without breaking customers. Public/CDN cache headers can be introduced later as a non-breaking improvement once we have real traffic data.
docs/PUBLIC_API_PLAN.md:394
- There is no T-093; the tier/rate-limit product-signoff work is T-091. This dangling task reference makes the open decision untraceable.
1. Rate-limit numbers per LFX membership tier (Gold, Platinum, etc.): TBD, pending product sign-off. Drives [T-093](#epic-e16--pre-launch).
docs/PUBLIC_API_PLAN.md:239
- The glossary says “phase” should be avoided in task descriptions, but E7–E13 are all named “Endpoint Migration Phase.” Rename these epics to Endpoint Groups so the task taxonomy follows the canonical language introduced by this PR.
### Epic E7: Endpoint Migration Phase 1: Development
docs/CONTEXT.md:34
- T-093 is not defined; the product signoff for tier hierarchy/rate-limit numbers is T-091 in PUBLIC_API_PLAN.md. Point the canonical glossary at the real task.
A named LFX membership level attached to an Organization that controls the rate-limit pool size. Known tiers in ascending order: Silver, Gold, Platinum (exact hierarchy and rate-limit numbers confirmed at T-093). In v1, tiers affect only rate limits; endpoint-level gating is reserved for future versions.
docs/CONTEXT.md:40
- The canonical Endpoint Group definition omits Overviews and Leaderboard even though the project plan defines seven groups. This incomplete list will cause tasks and generated docs to use inconsistent taxonomy.
A logical cluster of related endpoints released together as a unit (Development, Contributors, Popularity, Security, Collections). Each group maps to a Jira epic.
docs/architecture-review/03-context.md:44
- The canonical Endpoint Group definition omits Overviews and Leaderboard even though the project plan defines seven groups. Keep this human-readable glossary aligned with CONTEXT.md and the rollout plan.
A logical cluster of related endpoints released together (Development, Contributors, Popularity, Security, Collections). Each group maps to a Jira epic. Endpoints within a group are promoted through launch stages independently.
docs/architecture-review/01-overview.md:180
- This row is explicitly marked decided, but it appears under “Open Questions” whose introduction says every item is unresolved. Move the multi-org resolution to a resolved/notes section so reviewers do not treat it as pending architecture input.
| 2 | Multi-org Key Contact resolution (decided, 2026-05-19 review call + 2026-08-10): highest tier wins and the organization ID connected to that tier is returned; on a tie between orgs at the same tier, the first one returned by the Tier endpoint is used as the rate-limit pool key. | [T-015](../PUBLIC_API_PLAN.md#epic-e3--auth--rate-limiting-api-keys-via-lfx-self-serve) |
docs/architecture-review/01-overview.md:132
- All
#epic-eN--...links in this file contain an extra hyphen after the epic number; generated anchors use one hyphen after the colon is removed (for example,#epic-e15-key-management-entry-point-lfx-insights-frontend). Update each rollout/task link so the architecture overview is navigable.
Personal Access Tokens (what customers call their "API key") are created and managed entirely in the LFX Self-Serve App's Developer Settings. The LFX Insights frontend deep-links to that page from a `/settings/api-keys` placeholder ([E15](../PUBLIC_API_PLAN.md#epic-e15--key-management-entry-point-lfx-insights-frontend)); it does not implement create / list / revoke. Membership gating (only Key Contacts in member organizations can create keys) is enforced by LFX Self-Serve, not by Insights. What the customer pastes into their environment is the PAT itself, sent directly as `Authorization: Bearer lfi_...`. The Cloudflare Worker exchanges it for a short-lived Auth0-signed JWT on a cache miss (~10 min TTL), so there is no client-side token-swap call and no Insights-hosted token endpoint (per ADR-0006 and ADR-0015).
docs/architecture-review/02-decisions.md:116
- The target heading generates
#d5-datadog-metrics-strategy-custom-metrics-vs-apm-trace-metrics, not an anchor with two hyphens after “strategy,” so this decision-summary link is broken.
### Datadog: hybrid custom metrics + APM trace metrics: [PUBLIC_API_PLAN.md §3 D5 + §6](../PUBLIC_API_PLAN.md#d5-datadog-metrics-strategy--custom-metrics-vs-apm-trace-metrics)
docs/adr/0003-tolerant-reader-versioning.md:20
- The generated E16 heading anchor is
#epic-e16-pre-launch; the extra hyphen in this target leaves the promotion criterion link broken.
1. **Load test passes**: baseline req/s established, rate limiter validated under load ([T-090](../PUBLIC_API_PLAN.md#epic-e16--pre-launch)).
docs/adr/0006-pat-token-exchange-for-api-credentials.md:15
- This Decision states that variant 4b is selected, but the same accepted ADR later says 4a and 4b both remain under consideration. Separate the accepted PAT-exchange decision from the unresolved entitlement-placement choice so implementers do not treat 4b as final.
We will issue long-lived Personal Access Tokens from the LFX Self-Serve App and exchange them for short-lived Auth0-signed JWTs using Auth0 Custom Token Exchange, performed by a Cloudflare Worker in front of the Insights API. The exchange runs on a cache miss, not on every request. The Worker caches the exchanged JWT and the caller's entitlements for ~10 min. Membership org and tier are resolved by the Worker from a purpose-built LFX Tier endpoint and passed to the Insights API as trusted headers (variant 4b); the Insights API verifies the JWT and never sees the PAT.
docs/adr/0011-pagination-cursor-based.md:23
- This says cursor encoding can change without a breaking change, while ADR-0003, CONTEXT.md, and the project plan explicitly classify changing cursor encoding semantics as breaking. Opacity prevents clients from parsing the cursor, but the server must still accept outstanding v1 cursors or version the change.
The cursor is `base64url(JSON.stringify({ k: <last sort-key value>, id: <tiebreaker id> }))`. It is server-generated and server-opaque: clients must not parse, construct, or store cursors as structured data. They pass back the `nextCursor` value verbatim. Opacity means the server can change the internal encoding (add fields, switch format) without issuing a breaking change.
docs/adr/0016-vitepress-scalar-api-docs.md:14
- This again names a single
api/openapi.json, while T-037 requires per-version specs served at/v1/openapi.json. Define the versioned build artifact that Scalar consumes; otherwise this ADR and the implementation plan specify different sources of truth.
- The Scalar reference page loads `api/openapi.json` at build time.
docs/adr/0001-fastify-over-nestjs.md:1
- Adding these ADRs also requires replacing the
_none yet_row indocs/adr/README.mdwith an entry for every decision, per.claude/rules/adr-format.md:43-52. The index is unchanged, so none of the new ADRs are discoverable through the repository’s canonical ADR index.
# Fastify over NestJS for the public API service
docs/PUBLIC_API_PLAN.md:131
- This says the docs have their own subdomain, while T-030, ADR-0016, and the overview put them on the API host at
/docs. The topology must be decided explicitly: same-host serving also makes Scalar’s browser client same-origin and bypasses the CORS boundary described in ADR-0004.
**Decision: VitePress + Scalar under `api/docs/`.** Standalone VitePress site co-located with the API service. Scalar embedded for the interactive OpenAPI reference, reading the generated spec: the reference cannot drift. Deployed independently of the frontend with its own subdomain.
docs/adr/0016-vitepress-scalar-api-docs.md:9
- Serving Scalar on the same origin as
/v1makes its browser client exempt from CORS, contradicting ADR-0004’s requirement that v1 be unusable from browser JavaScript and inviting users to paste long-lived PATs into a browser UI. Record whether the Scalar client is disabled; otherwise serve docs from a separate, non-allowed origin.
- **`api/docs/` standalone VitePress + Scalar (chosen).** Docs live alongside the service they document. Scalar is embedded on the reference page and reads `api/openapi.json`. The reference cannot drift from the implementation. Served at `api.insights.linuxfoundation.org/docs`. Same host as the API. Fastify serves the static VitePress build under `/docs`, no extra subdomain needed.
docs/PUBLIC_API_PLAN.md:127
- Scalar’s built-in browser client conflicts with the server-to-server-only contract. Because the plan serves docs from the API origin, requests from
/docsto/v1are same-origin and CORS does not apply, so users could submit long-lived PATs from browser JavaScript despite ADR-0004. Disable Scalar’s API client for v1 or move docs to a separate origin that the API does not allow.
| **Scalar** ⭐ (OSS fallback) | OSS, "Stripe-like" reference UI: easily the prettiest of the OSS options; embeddable into anything (Vue/VitePress/Next/Hono); best-in-class OpenAPI rendering; built-in "try it" client; fast; well-funded team behind it. | Just a reference renderer: you bring your own narrative/guide layer (we'd marry it with VitePress for guides); smaller team than Stoplight; theming is configurable but less plug-and-play than Mintlify. |
docs/CONTEXT.md:26
- The plan defines the User as the API/identity principal and explicitly says usage is not invoiced; the Organization holds the paid membership tier. Calling the User the “billing principal” contradicts that model and can mislead downstream authorization/telemetry design.
The human account that owns one or more API Keys (PATs) and is the billing principal. Identified by the JWT `sub` claim.
docs/architecture-review/03-context.md:28
- The plan defines the User as the API/identity principal and explicitly says usage is not invoiced; the Organization holds the paid membership tier. Calling the User the “billing principal” contradicts that model.
The human account that owns one or more API Keys (PATs) and is the billing principal. Identified by the JWT `sub` claim.
docs/PUBLIC_API_PLAN.md:20
- The plan later commits to bundled membership with no usage invoicing (§9 decision 14), so “bill by tier” describes a billing capability that v1 explicitly will not implement. Frame observability around SLA and rate-limit operations instead.
This issue also appears on line 395 of the same file.
- Heavy observability (OTel → Datadog) so we can offer SLAs and bill by tier confidently.
docs/PUBLIC_API_PLAN.md:177
- The plan never reconciles these probe routes with ADR-0009’s “every request requires a valid API key” rule. Requiring a customer JWT for Kubernetes liveness/readiness makes pod health depend on the Worker/Auth0 path and requires secret-bearing probe configuration; define
/health/*as a network-restricted probe exception or specify a separate probe-auth contract.
- **T-006** Standard health endpoints: `/health/live`, `/health/ready` (TB ping, Redis ping, PG ping).
docs/adr/0009-api-key-required-for-all-requests.md:3
- “All endpoints” also covers the planned Kubernetes health routes,
/docs, and versioned OpenAPI artifacts, but no authentication contract is defined for those non-data surfaces. A global API-key hook would reject probes and make documentation inaccessible without a customer credential. Scope this floor to customer API routes and explicitly define network/auth policy for health and documentation routes.
All endpoints, including those that expose public project data (Endpoint Groups 1–4), require a valid API key. There is no unauthenticated access path. A missing or invalid key returns 401 immediately. We chose the auth-floor approach because: (1) rate limiting and abuse prevention require a stable identity to enforce per-org quotas; (2) attribution (knowing which orgs use which endpoints) is essential for prioritizing the roadmap and justifying infrastructure cost; (3) anonymous access complicates the future tier-gating mechanism. The cost is a higher onboarding barrier (users must create an API key before their first request). This can be revisited if adoption data shows the friction is significant.
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (15)
docs/adr/0003-tolerant-reader-versioning.md:37
- The documented header values are not standards-compliant. RFC 9745 requires
Deprecationto be a Structured Field Date (@<unix-seconds>), while RFC 8594 requiresSunsetto be an HTTP-date rather than ISO-8601; clients implementing those RFCs may ignore the current values.
2. **Add response headers** on the affected endpoint: `Deprecation: true` and `Sunset: <ISO-8601 date>` (the earliest date we will remove it), plus `Link: <migration-guide-url>; rel="deprecation"` pointing to the migration guide.
docs/PUBLIC_API_PLAN.md:236
Deprecation: trueand an unspecified/ISO-style Sunset date do not implement the standardized headers. Define T-038 using RFC 9745's Structured Field Date forDeprecationand RFC 8594's HTTP-date forSunset, otherwise standards-aware tooling can ignore them.
- **T-038** Deprecation/Sunset header support (`Deprecation: true`, `Sunset: <date>`, `Link: <docs>; rel="deprecation"`).
docs/architecture-review/02-decisions.md:73
- This summary repeats invalid wire formats: RFC 9745 does not allow
Deprecation: true, and RFC 8594 requires an HTTP-date forSunset. Keeping this summary aligned with ADR-0003 prevents implementers from copying the invalid examples.
When a field, parameter, or endpoint needs to eventually be removed, the deprecation process is: (1) annotate it as `DEPRECATED:` in the TypeBox schema so it surfaces in the OpenAPI spec; (2) add `Deprecation: true`, `Sunset: <date>`, and `Link: <migration-guide>; rel="deprecation"` response headers on every response from that route; (3) publish a changelog entry and notify known consumers; (4) honour the sunset window. Removal before the `Sunset` date is a contract violation; (5) do the actual removal in `/v2`, not `/v1`. Full details in [docs/adr/0003](../adr/0003-tolerant-reader-versioning.md).
docs/adr/0013-origin-cache-only-private-cache-control.md:8
private, max-age=0does not mean clients do not cache: private caches may store the response, but it is immediately stale and must be revalidated. Either useno-storeto enforce the stated origin-only/no-client-storage contract, or revise the contract to explicitly allow private storage with revalidation.
Clients and intermediary proxies/CDNs do not cache responses (`Cache-Control: private, max-age=0`). We chose origin-only caching over public HTTP caching because: (1) some endpoints are user-scoped (Collections) where a shared CDN cache would be a security error; (2) origin Redis gives us a single TTL knob tunable without a contract change. Public cache headers can be introduced later as a non-breaking improvement once per-endpoint analysis is done. Engineers must not add `public` or `s-maxage` headers without a deliberate review.
docs/PUBLIC_API_PLAN.md:385
- This contract overstates what
private, max-age=0guarantees. It permits private client caches to store responses and only forces revalidation; useno-storeif customers truly must not cache, or document the intended revalidation behavior.
18. **Caching contract (v1):** origin-side Redis cache only (two-tier TTL per ADR-0013: 24h for stable data, 1h for time-series analytics). All responses set `Cache-Control: private, max-age=0`. Customers do not cache, intermediaries do not cache. Lets us tune TTLs without breaking customers. Public/CDN cache headers can be introduced later as a non-breaking improvement once we have real traffic data.
docs/architecture-review/02-decisions.md:103
private, max-age=0still permits client-side storage, so “Clients ... do not cache” is inaccurate. Align this summary with ADR-0013 after deciding betweenno-storeand private storage with mandatory revalidation.
All responses carry `Cache-Control: private, max-age=0`. A Redis cache with two TTL tiers lives at the origin: 24h for stable data (project lists, leaderboards, categories), 1h for time-series analytics. Clients and CDNs do not cache. This keeps a single TTL knob we can tune without a contract change, and avoids accidental public caching of org-scoped Collection responses.
docs/PUBLIC_API_PLAN.md:397
- The verification path is not identical under 4a: org/tier move from unsigned Worker headers into signed JWT claims, so T-017 must parse different inputs and the header-spoofing/origin-proof requirements in T-015c change. Either settle on 4b before defining those tasks or make the affected tasks explicitly conditional for both variants.
2. **Variant 4a vs 4b: where does tier resolution live?** 4b (suggested direction, per ADR-0006) has the Cloudflare Worker resolve org and tier from an LFX Tier endpoint and pass them as headers; 4a has the PAT service enrich them into the JWT. The PAT, exchange, and verification path are identical either way, so this can be settled without reworking the rest. Insights stewards the call, with DevOps input; confirmed at T-015. See [ADR-0006](adr/0006-pat-token-exchange-for-api-credentials.md).
docs/adr/0011-pagination-cursor-based.md:9
- Keyset pagination does not guarantee “no duplicates or missed entries” when the selected sort key itself changes between requests. The plan explicitly permits mutable analytics sorts such as
commits_desc, so a row can cross the cursor boundary during iteration. Require an immutable ordering/snapshot watermark for exact iteration, or qualify this guarantee.
1. **Stability under mutations: no duplicates or missed entries.** Offset pagination produces corrupt iteration when the underlying set changes between page fetches. Concretely: if an item is inserted between fetching page N and page N+1, every subsequent row shifts one position forward. The item that was the first entry of page N+1 slides back into the last position of page N. We already fetched that page, so it is silently skipped. The reverse happens on deletion: the item that was the last entry of page N drops into the first position of page N+1, so it appears in both pages. A cursor anchors to a row's sort-key value rather than its offset, so insertions and deletions between calls never affect which rows the caller sees next. This API serves analytics over data that grows continuously (commits, contributors, vulnerabilities), making offset instability a practical concern, not a theoretical one.
docs/adr/0019-opentelemetry-instrumentation.md:13
- An inbound distributed trace can legitimately contain multiple API requests, all sharing the same trace ID. Those responses would therefore expose the same
requestId, so support cannot uniquely identify the failing request as this contract promises. Keep the trace ID for correlation, but add a request-unique value (for example the server span ID or a generated request ID) to the error contract.
- Error envelope `requestId` field = OTel trace ID (32-char lowercase hex). This is the customer-facing ID for support tickets. It correlates directly to logs and APM traces in Datadog without translation.
docs/PUBLIC_API_PLAN.md:379
- This absolute authentication rule contradicts ADR-0009's unauthenticated
/health/*,/docs, and/v1/openapi.jsonsurfaces. Scope the decision to customer data routes so implementers do not register authentication globally.
12. **Authentication floor:** every request requires a valid API key. No anonymous access path. 401 on missing/invalid key, full stop.
docs/architecture-review/02-decisions.md:37
- This says there is no unauthenticated path, but ADR-0009 explicitly exempts health probes, docs, and the OpenAPI document. State the customer-data-route scope here to keep the decision summary from reintroducing the global-auth design that ADR-0009 rejected.
All endpoints, including those serving public project data, require a valid API key. There is no unauthenticated path. A missing or invalid key returns 401 immediately. This is intentional: rate limiting requires a stable identity, and attribution data is essential for roadmap prioritization.
docs/architecture-review/03-context.md:79
- This canonical definition omits curated/system Collections and implies every caller must prove ownership, conflicting with
docs/CONTEXT.md:62and ADR-0007, where public Collections are available to any valid key and only private Collections require owner matching. Align the definition so it does not lead implementers to deny public or curated Collections.
**Collection**
A user-curated named group of projects, stored in Postgres. The only Endpoint Group (Group 6) that requires a per-request permission check. Callers must prove they have access to the specific Collection they are querying.
.claude/rules/adr-format.md:22
- The relaxed rule still mandates rejected alternatives and rationale, but ADR-0010 and ADR-0018 added in this PR do not state alternatives or why they were rejected. Either add that reasoning to those ADRs or relax this requirement as well; otherwise the new ADR set still violates its own repository rule.
2. Body prose that covers the context, the decision itself, and the
alternatives considered with why they were rejected
docs/adr/README.md:21
- Every new row except ADR-0006 leaves
StatusandDateblank, so the canonical index cannot tell reviewers whether these records are proposed or accepted, or when they were decided. Populate those columns consistently; the architecture overview currently says the overall review is still pending, so silently treating them as accepted would also be misleading.
| [ADR-0001](./0001-fastify-over-nestjs.md) | Fastify over NestJS for the public API service | | |
| [ADR-0002](./0002-api-at-repo-root.md) | Public API service lives at `/api` (repo root), not inside `workers/` | | |
| [ADR-0003](./0003-tolerant-reader-versioning.md) | v1 contract: tolerant-reader / additive-only changes within a version | | |
| [ADR-0004](./0004-server-to-server-cors-deny.md) | v1 is server-to-server only; CORS denies all browser origins | | |
| [ADR-0005](./0005-tiers-control-rate-limits-only.md) | Tiers control rate limits only in v1; no per-endpoint feature gating | | |
docs/PUBLIC_API_PLAN.md:384
- Adding a new success status code to an existing operation is not generally additive: callers commonly branch on the documented status (for example
200versus202), and the new status may change response or completion semantics. Remove this from the within-version allowance, or constrain it to new endpoints.
17. **Versioning semantics ("breaking change" definition):** **tolerant-reader / additive-only**. Within a version, allowed: adding response fields, adding optional query params, adding endpoints, expanding accepted enum INPUT values, adding new error codes, adding new success status codes. Requires a major version bump: removing/renaming a response field, changing a field's type, making an optional input required, narrowing accepted input values, removing an endpoint, changing the error envelope shape, changing default or max pageSize, or changing the cursor encoding semantics. **Customers commit to ignoring unknown response fields** (documented prominently). Matches Stripe/GitHub/Google.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (11)
docs/adr/0003-tolerant-reader-versioning.md:37
- The documented wire values are not valid for these standard headers. RFC 9745 defines
Deprecationas a Structured Field Date (for example,@1735689600), not the booleantrue, and RFC 8594 requiresSunsetto use HTTP-date/IMF-fixdate rather than ISO-8601. Implementing this text would emit headers that standards-aware clients cannot parse.
2. **Add response headers** on the affected endpoint: `Deprecation: true` and `Sunset: <ISO-8601 date>` (the earliest date we will remove it), plus `Link: <migration-guide-url>; rel="deprecation"` pointing to the migration guide.
docs/PUBLIC_API_PLAN.md:236
- This task specifies invalid wire formats:
Deprecation: trueis not an RFC 9745 Structured Field Date, andSunsetmust be an RFC 8594 HTTP-date. Specify the formats explicitly so the implementation and generated contract are interoperable.
- **T-038** Deprecation/Sunset header support (`Deprecation: true`, `Sunset: <date>`, `Link: <docs>; rel="deprecation"`).
docs/architecture-review/02-decisions.md:73
- The summary repeats invalid standard-header values.
Deprecationrequires an RFC 9745 Structured Field Date, andSunsetrequires an RFC 8594 HTTP-date;trueand an unspecified/ISO date will not be parsed correctly by standards-aware clients.
When a field, parameter, or endpoint needs to eventually be removed, the deprecation process is: (1) annotate it as `DEPRECATED:` in the TypeBox schema so it surfaces in the OpenAPI spec; (2) add `Deprecation: true`, `Sunset: <date>`, and `Link: <migration-guide>; rel="deprecation"` response headers on every response from that route; (3) publish a changelog entry and notify known consumers; (4) honour the sunset window. Removal before the `Sunset` date is a contract violation; (5) do the actual removal in `/v2`, not `/v1`. Full details in [docs/adr/0003](../adr/0003-tolerant-reader-versioning.md).
docs/adr/0011-pagination-cursor-based.md:9
- The insertion/deletion outcomes are reversed. An insertion before the offset boundary shifts the former last row of page N onto page N+1, causing a duplicate; a deletion shifts the former first row of page N+1 into the already-passed page N, causing a skip. Correcting the example is important because it is the primary correctness rationale for this ADR.
1. **Stability under mutations: no duplicates or missed entries.** Offset pagination produces corrupt iteration when the underlying set changes between page fetches. Concretely: if an item is inserted between fetching page N and page N+1, every subsequent row shifts one position forward. The item that was the first entry of page N+1 slides back into the last position of page N. We already fetched that page, so it is silently skipped. The reverse happens on deletion: the item that was the last entry of page N drops into the first position of page N+1, so it appears in both pages. A cursor anchors to a row's sort-key value rather than its offset, so insertions and deletions between calls never affect which rows the caller sees next. This API serves analytics over data that grows continuously (commits, contributors, vulnerabilities), making offset instability a practical concern, not a theoretical one.
docs/adr/0019-opentelemetry-instrumentation.md:15
- An inherited trace ID is not a unique request identifier. One distributed trace can contain multiple calls to this API, and callers control/reuse the inbound
traceparent, so several responses can expose the samerequestId; support cannot reliably identify one request from that value. Keep the trace ID for correlation, but expose a server-generated request ID or a request-specific span identifier as well.
Auto-propagation makes a separate ULID-based request ID redundant: every request already has a 128-bit OTel trace ID generated at the inbound HTTP span (or honoured from an inbound `traceparent`). We use that trace ID as our request identifier:
- Error envelope `requestId` field = OTel trace ID (32-char lowercase hex). This is the customer-facing ID for support tickets. It correlates directly to logs and APM traces in Datadog without translation.
- Log lines include `trace_id` and `span_id` (OTel hex format, 128-bit and 64-bit). Datadog's APM ingester recognises OTel-format IDs natively. No parallel `dd.trace_id` / `dd.span_id` fields are emitted. A pino mixin can re-add `dd.trace_id` without contract impact if a future Datadog regression breaks UI joins.
- Inbound `traceparent` is honoured (caller's trace continues across the boundary).
docs/PUBLIC_API_PLAN.md:197
- T-015 explicitly leaves 4a versus 4b unresolved, but this implementation task unconditionally assigns tier resolution and entitlement headers to the Worker (4b). If T-015 selects 4a, this scope and the dependent T-015c/T-017 work are wrong. Make the task conditional on the T-015 outcome or define variant-specific follow-up tasks.
- **T-015b** Build the PAT exchange path: PAT service work in Self-Serve (generation, salted-hash storage, rename/revoke, audience prefixing, Key Contact / entitlement validation at issuance, Auth0 validation callback) and the Cloudflare Worker that detects the `lfi_` prefix, calls Auth0 `POST /oauth/token` with `grant_type=urn:ietf:params:oauth:grant-type:token-exchange` plus the registered `subject_token_type`, the target `audience`, and the Worker's client credentials, resolves org and tier, **strips or overwrites any client-supplied `x-tier` / organization header**, and forwards to the origin. Exchange runs on a cache miss (~10 min), not per request. No Insights-hosted token endpoint.
docs/PUBLIC_API_PLAN.md:211
- The API is required to emit
api_key_id, but the documented request contract gives it only JWT identity plus org/tier headers; the claim table in ADR-0015 has no PAT ID, and Insights never receives the PAT. Therefore T-023 cannot populate this attribute. Define a trusted PAT-ID claim/header from the exchange path (and add it to T-015/T-017), or removeapi_key_idfrom the telemetry contract.
- **T-023** Integrate OpenTelemetry SDK (`@opentelemetry/sdk-node`): HTTP auto-instrumentation, Postgres auto-instrumentation, custom spans around Tinybird calls. W3C TraceContext propagator (default). Span attributes carry high-cardinality dimensions (`enduser.id`, `api_key_id`, `bucket_id`, `pipe`, numeric `status_code`). Per ADR-0019.
docs/adr/0013-origin-cache-only-private-cache-control.md:8
private, max-age=0does not mean clients do not cache: it permits private caches to store the response immediately stale and revalidate it;privateonly prevents shared-cache storage. This contradicts the origin-only/no-client-cache contract. Useno-storeif storage must be prohibited, or update the stated contract to allow private revalidation.
Clients and intermediary proxies/CDNs do not cache responses (`Cache-Control: private, max-age=0`). We chose origin-only caching over public HTTP caching because: (1) some endpoints are user-scoped (Collections) where a shared CDN cache would be a security error; (2) origin Redis gives us a single TTL knob tunable without a contract change. Public cache headers can be introduced later as a non-breaking improvement once per-endpoint analysis is done. Engineers must not add `public` or `s-maxage` headers without a deliberate review.
docs/adr/0009-api-key-required-for-all-requests.md:11
- The plan defines one OpenAPI artifact per major version, but this exemption names only v1. When
/v2/openapi.jsonis added it would fall outside the documented unauthenticated surfaces, preventing docs/tooling from consuming it consistently. Make the exemption version-generic.
- `/v1/openapi.json`: the machine-readable spec the docs and tooling consume.
docs/PUBLIC_API_PLAN.md:203
X-RateLimit-*is a de facto legacy convention, not a standard header family, so labeling it “Standard” bakes a misleading contract into a new API. Either call these conventional compatibility headers or select and document the current IETFRateLimit/RateLimit-Policyfield format.
- **T-020** Standard rate-limit response headers (`X-RateLimit-*`, `Retry-After`) and 429 envelope.
docs/architecture-review/03-context.md:79
- This definition omits curated/system Collections and incorrectly implies every Collection requires callers to prove ownership. The canonical context says curated Collections have
ssoUserId = null, public Collections are visible to any valid key, and only private Collections use the creator check. Align this summary so implementers do not apply ownership gating to public or curated data.
A user-curated named group of projects, stored in Postgres. The only Endpoint Group (Group 6) that requires a per-request permission check. Callers must prove they have access to the specific Collection they are querying.
main