Repository navigation
Conversation
PostHog Review · Deep · reviewed
|
| Step | Status | Result |
|---|---|---|
| Prepare the diff | done | 9 files, 3 chunks |
| Pick perspectives | done | 3 perspectives |
| Run review passes | done | 3 issues (+2 blind-spot) |
| Merge overlapping findings | done | 5 → 5 |
| Validate findings | done | 2 kept, 3 dismissed |
| Publish | done | 2 posted (view review) |
There was a problem hiding this comment.
See this review and its findings in PostHog Review.
dustinbyrne
left a comment
There was a problem hiding this comment.
The server-side contract is grounded in the existing implementations and backend. One optional clarification would preserve configured host prefixes in the containment wording.
AI-assisted review.
|
|
||
| ### Requirement: Flag key containment | ||
|
|
||
| The flag key SHALL NOT change the path outside its one segment, the query, or the fragment of the request. For every flag key, the path of a request that the SDK sends SHALL be `/api/projects/@current/feature_flags/{one segment}/remote_config`, and its query SHALL be `token={project token}`. The SDK SHALL meet this in one of two ways: it encodes the key so that it stays one path segment, or it refuses the key and sends no request. A refused key gives no result, and the SDK reports it as it reports a failure in "Request failure". |
There was a problem hiding this comment.
[P2] Preserve the configured host’s base path in the containment rule
Severity: MINOR · Disposition: OPTIONAL · Relationship: introduced documentation inconsistency.
Location: openspec/specs/get-remote-config-payload/spec.md:48, RIGHT; matching archived delta openspec/changes/archive/2026-11-13-add-get-remote-config-payload/specs/get-remote-config-payload/spec.md:25.
With host = https://analytics.example.com/posthog, the Request requirement at line 38 preserves /posthog, but line 48 permits only a path beginning /api/…. A prefix-preserving implementation cannot satisfy both literally:
Request :38:
/posthog/api/projects/@current/feature_flags/test-flag/remote_config
Containment :48:
/api/projects/@current/feature_flags/test-flag/remote_config
An ordinary server caller using that configured host and await client.getRemoteConfigPayload('test-flag') reaches this boundary. At Node 480395c443c5916d83b8c8fd8f6084f1ab244db2, packages/core/src/posthog-core-stateless.ts:443–453 preserves the configured prefix, and packages/node/src/client.ts:3410 appends the endpoint. The design-pinned Node helper at 9c9da5bf7a36842a0bea1b05dfd0bcfbe0714f92 behaves the same (packages/node/src/client.ts:3232). Baseline openspec/specs/remote-config/spec.md:166,178–181 at 57407963789cfc8ae411cf1895c09b1f7f8b575c also explicitly recognizes prefixed hosts; its bootstrap endpoint does not govern this getter, but establishes that hosts need not be origin-only.
A future port or containment assertion could discard a legitimate reverse-proxy prefix or incorrectly reject a prefix-preserving request. No existing runtime regression or actual prefix-dropping implementation was demonstrated. The capability is absent at base; the inconsistency is between this PR’s two new normative expressions.
Consider defining containment as the configured host’s base-path prefix followed by /api/projects/@current/feature_flags/{one segment}/remote_config, keeping everything outside the key segment immutable. Mirror the wording in the archived delta. Consider adding unbound prefixed-host cases for a plain key and a traversal key, retaining origin-only cases as controls.
Verification: Deterministic source/contract comparison confirmed the two path expressions disagree; the local document check passed. This was not an SDK reproduction. Runtime execution is unnecessary to establish the documentation inconsistency; adapter-level HTTP verification belongs with future harness bindings.
Problem
posthog-node, posthog-python, posthog-ruby, posthog-go and posthog-dotnet can each read the payload of a remote config feature flag by flag key, with a secret key and no distinct id. They disagree on a missing secret key, the return type, flag key escaping, how a failure reaches the caller, and what a non-2xx response returns.
sdk-specs has no contract for the method, so the server SDKs that still lack it (
posthog-serverin posthog-android, posthog-php, posthog-elixir, posthog-rs) have nothing to implement against. Theremote-configcapability covers the client/configbootstrap, which is a different feature.Part of PostHog/posthog-android#577
Changes
Adds the
get-remote-config-payloadcapability for server SDKs. The reference surface isgetRemoteConfigPayloadin posthog-node, and the request isGET {host}/api/projects/@current/feature_flags/{key}/remote_config?token={project token}with the existing secret key as a Bearer token.The spec covers:
One requirement is mandatory although posthog-node doesn't meet it: a flag key can't change the path outside its one segment, the query, or the fragment of the request. The request carries the secret key, and a raw key like
../../../users/@me/#sends it to another API route. An SDK encodes the key as one path segment, or refuses it.No shipped SDK contains every key. posthog-node, posthog-python (sync path), posthog-ruby and posthog-go put the raw key into the path, and posthog-dotnet and the posthog-python async path don't contain
.and...design.mdhas the pinnedfile:linefor each, and the fixes are follow-ups in those repos.design.mdalso records one gap in posthog-node against "Request failure": if the connection fails during the body read, the call rejects (packages/node/src/client.ts:2192).Three more requirements go beyond posthog-node and are recommendations (
SHOULD):posthog-node has none of these three behaviors, and nothing changes in posthog-js with this spec. The empty body matters because PostHog answers a flag that has no payload with status 200 and an empty body, and
response.json()in posthog-node rejects on it. Every shipped SDK stays conformant on these three.design.mdrecords which SDKs already follow each recommendation, with pinnedfile:linereferences.Their acceptance scenarios carry
@flag_key_encoding_capable,@failure_status_capableand@empty_body_capable, so an SDK that doesn't follow a recommendation can skip them.Left undefined on purpose, and listed as open decisions in
design.md:.or..Also adds
acceptance/public/get-remote-config-payload.feature. It carries no@sdk:servertag because no harness binding exists yet.The change is applied and archived, and
README.mdandopenspec/project.mdlist the capability.remote-configand the compliance files are unchanged.Testing
openspec validate --specs --strictpasses.