Select one feature flag provider at compile time

[TheSentinel454]

Why

Buzz server components need one typed feature-flag contract without forcing the open-source build to depend on LaunchDarkly. A staging or production relay must not be able to choose the wrong provider through runtime configuration.

What

• Adds typed boolean and signed i64 flags with static, environment, and compile-gated LaunchDarkly evaluators.
• Defines an executable relay artifact contract that compiles exactly one provider.
• Rejects zero or multiple relay provider features at compile time.
• Runs the LaunchDarkly feature tests and the provider compile matrix in the unit CI lane and fallback test script.

How

buzz-feature-flags stays provider-neutral. The future relay composition root will expose one build path per artifact feature: static-feature-flags, environment-feature-flags, or launchdarkly-feature-flags. Runtime configuration carries only settings for the selected provider. It never carries a provider name, fallback, or precedence rule.

The crate still has no relay consumer. A locked compile fixture proves each singleton builds and every zero or multi-provider combination fails until the relay adopts this contract.

This removes the proposed runtime ProviderMode and leaves one provider, one construction path, and one evaluator per artifact.

Risk

This PR still has no relay or app consumer, so it changes no user-visible behavior. Risk is limited to the new feature-flag crate, its compile contract, and its unit-test lane.

Testing

An external Rust consumer at 30cf968bfe85e82980fda7e63f5bebe649ca939f exercised six environment-evaluator scenarios and 59 assertions. The compile-time correction adds no live consumer to exercise manually.

Bigger picture

When buzz-relay adopts this crate, its build manifests must map each distribution to one provider feature. That consumer PR must verify live Relay Proxy reachability, secret ingestion, lifecycle ownership, and user-visible behavior.

Generated with Codex

[github-actions]

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

• Exact PR diff: 797012ff01a6d499959b45ed2e56f7927c6a4d6b...b166f66c31a22f957f8dbfd5896c055738d51a91
• Model: gpt-5.6-sol

💡 Click "edited" above to see earlier reviews for this PR.

---

Review Summary

Overall Risk: MEDIUM

The new feature-flag infrastructure has tenant-targeting and transport weaknesses, plus an integer precision bug. It is not yet wired into the relay, so these issues are latent until adoption.

Findings

[MEDIUM] Actor targeting is not scoped to its community

• Category: Isolation
• Location: crates/buzz-feature-flags/src/launchdarkly.rs:278 (source)
• Description: The LaunchDarkly pubkey context key contains only the actor public key. LaunchDarkly targets each context kind independently, so a rule targeting this pubkey matches the same actor in every community; merely including a separate community context does not conjunct it with the pubkey target.
• Impact: An actor-specific rollout intended for one community can activate in every other community containing that actor, expanding canary failures and violating the community-scoped evaluation contract.
• Recommendation: Key the actor context with both community ID and pubkey, or expose only a composite actor-within-community context. Add a test proving a pubkey target in one community does not match the same pubkey in another.

[MEDIUM] Relay proxy configuration permits cleartext SDK traffic

• Category: Cryptography
• Location: crates/buzz-feature-flags/src/launchdarkly.rs:216 (source)
• Description: Relay proxy validation accepts arbitrary HTTP endpoints, and the tests explicitly treat a non-loopback HTTP endpoint as valid. The server SDK sends authenticated configuration and event traffic, including its SDK credential, through this endpoint.
• Impact: An attacker able to observe or modify traffic to such a proxy can capture the SDK key, inspect flag configuration, or inject flag data into the process.
• Recommendation: Require HTTPS for non-loopback endpoints. If cleartext transport is needed for local development or a secured service mesh, require a separate explicit insecure option rather than accepting it by default.

[LOW] Large integer variations can be silently rounded

• Category: Reliability
• Location: crates/buzz-feature-flags/src/launchdarkly.rs:175 (source)
• Description: Integer evaluation first asks the SDK for an f64 and only then checks whether it is integral. Values such as 9007199254740993 have already rounded to 9007199254740992 before strict_f64_to_i64 runs, so the fractional and range checks accept the wrong integer.
• Impact: A configured i64 flag can select an unintended limit, version, or implementation without falling back to its safe default.
• Recommendation: Read and validate the original numeric representation if the SDK exposes it, or reject/document integer variations outside the exactly representable IEEE-754 range and enforce that range in the API.

Notes

• The crate is currently infrastructure-only and has no relay or database consumer in this PR, so the reported production impacts begin when the adapter is integrated.

---

_{Generated by Codex Security Review |
Requested by: @TheSentinel454 |
Workflow run}

[wpfleger96]

🤖 Combined review from Paul + Thufir — both reviewed independently, findings reconciled and deduped, per the beekeepers-channel request. Paul verified Thufir's SDK-source claims independently against the locked launchdarkly-server-sdk 3.2.0 / evaluation-crate sources before including them. All paths below are under crates/buzz-feature-flags/.

OSS-genericity: passes. Default build has zero LaunchDarkly dependency (optional = true, module compile-gated). Public contract uses Buzz community IDs/pubkeys, generic env configuration, static defaults. Proxy/key settings are caller-supplied; no Block-specific hosts, credentials, auth, or deployment wiring in the added files. LaunchDarkly support is vendor-specific but not Block-specific, and it's optional.

IMPORTANT findings

1. Fractional flag values silently become integers (Correctness). src/launchdarkly.rs:149–155 delegates to SDK int_variation(), and the locked SDK's f64_to_i64_safe does f as i64 — 1.99 → 1, -1.99 → -1 (verified at util.rs). This violates the typed-integer/default contract in src/lib.rs:64–68 and is inconsistent with the environment provider, which rejects 12.5. A malformed remote value can select a valid-but-unintended path instead of the declared safe default. Inspect the numeric value, reject non-integral/out-of-range, fall back to the declared default; add fractional ± and boundary tests.

2. Malformed relay-proxy endpoint can panic instead of returning LaunchDarklyInitError (Correctness). src/launchdarkly.rs:102–113 checks only for emptiness. relay_proxy() sets events_base_url, and the default event processor does Uri::from_str(format!("{}/bulk", events_base_url)).unwrap() (verified at processor_builders.rs) — a nonempty malformed endpoint like https://relay proxy.example panics past the ?. Validate the endpoint as an HTTP(S) URI before handing it to the SDK and return a sanitized typed error; test the public constructor with malformed endpoints.

3. Unit tests leave real analytics egress enabled (Correctness / test isolation). The test helper (src/launchdarkly.rs:194–205) substitutes TestData for the flag source only; the SDK still builds its default HTTP event processor targeting https://events.launchdarkly.com, evaluations queue analytics, and close() flushes them synchronously (verified at config.rs and client.rs). The unit lane therefore attempts external vendor traffic with a dummy key and couples test latency to network behavior. Keep TestData but install NullEventProcessorBuilder for evaluation tests (not SDK offline mode, which would bypass the positive cases).

4. Checked-in fixture Cargo.lock couples the unit lane to the whole workspace dependency tree (Maintainability). tests/fixtures/relay-feature-selection/Cargo.lock (~2,800 lines) locks the full transitive tree of buzz-core (bitcoin, aws-lc-sys, …), and the contract test runs nested cargo check --locked against it. Any PR changing dependency requirements anywhere in that tree makes the nested lockfile stale and fails the unit lane on unrelated PRs with a confusing nested-cargo error. Add a documented regeneration recipe (e.g., a Justfile target) and a test assertion message pointing at it — or shrink the fixture's dependency surface.

MINOR findings

• README composition-root example never starts the LaunchDarkly client (README.md:141–156): show start_with_default_executor_and_wait() and the initialization-failure policy; construction alone returns defaults indefinitely.
• Contract-test cost: the selection test compiles 8 nested cargo variants (including native aws-lc-sys builds) inside the unit lane on a fresh temp CARGO_TARGET_DIR. Worth measuring; move to a dedicated lane or cache if it dominates.
• Env-name collisions are documented but unenforced: a-b and a_b both normalize to BUZZ_FEATURE_FLAG_A_B. Fine while there are no consumers; when a flag registry appears, add a uniqueness assertion.
• Exact-one provider enforcement lives only in the fixture/README until the relay consumer PR copies the compile_error! guard; the README should state that copying it verbatim is a hard requirement of the consumer PR.

Solid: typed flags with declared safe defaults on every failure path (missing, wrong type, non-Unicode, provider failure), SDK key and env values redacted in Debug, sanitized diagnostics without raw values or env names, poisoned-mutex recovery, and community-first EvaluationContext matching the multi-tenant model, with same-actor-two-communities isolation tested. CI at head 892eefd1d: 60 pass / 0 fail, all 37 crate tests executed including the compile matrix.

[wpfleger96]

🤖 Re-review by Thufir at 657e6279b0ae21a7460203c4cf05da7c8b8814e6 (pass 2). The three SDK findings are addressed: integer evaluation rejects fractions/out-of-range values and preserves the exact declared default on SDK fallback; the public constructor validates proxy HTTP(S) URIs before SDK construction; and the evaluation-test helper installs NullEventProcessorBuilder without disabling positive flag evaluation.

The startup example now awaits initialization and states a fail-closed policy. The README also makes exact-one guards mandatory for the future consumer and explicitly defers normalized-name uniqueness to a future registry. OSS-genericity still passes; there is no production relay/DB consumer in this PR.

One part of previous finding 4 remains: crates/buzz-feature-flags/tests/relay_feature_selection_contract.rs:109–116 still omits the regeneration recipe from the actual stale-lockfile failure. Command::output() returns Ok(Output) when Cargo exits unsuccessfully, so the new unwrap_or_else message at lines 44–50 is not reached by cargo check --locked rejecting a stale lock. The missing-file assertion is not reached either when the stale file exists. The README recipe is useful; please also include FIXTURE_LOCK_REGEN_RECIPE in the failed valid-case assertion (and unexpected-error diagnostic for invalid cases). This finishes the already-requested recovery guidance without changing the fixture architecture. Validate by making the fixture lock stale and checking that the failing test reports the exact regeneration command.

Evidence: CI unit execution ran all 50 crate tests successfully; the eight-case compile matrix took 52.197 seconds. Latest check snapshot: 54 successful, 28 skipped, no failures. I inspected CI rather than duplicating its suite locally.

Approval remains pending the recovery-path correction and an independent public-constructor/start/evaluate/close loopback probe of proxy routing, initialization failure, and analytics shutdown. That probe has been requested; I do not yet have a runtime result. The separate Codex security-review comment is also marked stale for this exact head; this review does not claim that check is current.

[wpfleger96]

🤖 I’ve reconciled Gurney’s independent public-boundary probe with the source at 657e6279b0ae21a7460203c4cf05da7c8b8814e6. This completes pass 2; I’m requesting changes for the remaining endpoint-routing defect.

The valid lifecycle holds: root and /proxy/ endpoints initialize, streamed integer updates change 42 to 84, fractional/missing flags preserve exact defaults, HTTP 401 and stalled initialization return the expected errors, and close() flushes analytics to the configured proxy and disconnects the stream. The probe used the public constructor/start/evaluate/close API with a dummy key and loopback server; non-loopback connections were sandbox-denied. I inspected the saved consumer, recorder, and raw results and independently traced the locked SDK source; I did not rerun Gurney’s probe or CI suites.

IMPORTANT — Correctness: please reject unsupported query/fragment base endpoints in validate_relay_proxy_endpoint. The constructor currently accepts both forms, but the SDK appends service suffixes as strings. Actual recorded requests were:

Configured endpoint	Stream request	Analytics request
http://127.0.0.1:<port>/?route=proxy	GET /?route=proxy/all	POST /?route=proxy/bulk
http://127.0.0.1:<port>/#proxy	GET /	POST /

Both constructors returned success, then initialization failed against the proxy routes. This is not the original panic recurring, but the validator still admits inputs that cannot preserve the service paths, sending both flag and analytics traffic to unintended routes. The simplest fix is a sanitized typed rejection before SDK construction, preserving valid path prefixes. Check fragments in the original input: locked http 1.4.2 discards them during parsing, so inspecting only the parsed URI loses the evidence. Add public-constructor regression cases for queries/fragments plus positive root/path-prefix cases, and rerun the loopback routing probe after the fix.

The source matches the runtime evidence: SDK 3.2.0 service_endpoints.rs:126–130 only trims trailing slashes; data_source.rs:81 and processor_builders.rs:102 append /all and /bulk. I verified the inspected SDK and HTTP source files against crate archives matching this head’s lockfile checksums.

MINOR — Nit: the same validator accepts :notaport and :99999; explicitly validate supplied ports. Their startup timeouts were observed under deny-all-network containment, so I’m not claiming what uncontained connections would do.

The fixture-lockfile diagnostic correction from my earlier review also remains outstanding at this unchanged head. My earlier statement that the three SDK findings were addressed was too broad: the original malformed-URI panic cases are fixed, but endpoint validation is not yet complete.

CI remains green and the separate security-review comment remains stale for this head. No real-vendor/TLS or relay-integration claim is made; this crate still has no production relay consumer. OSS-genericity remains sound. No product source was changed during either review lane.

[TheSentinel454]

Addressed both remaining pass-2 findings at exact head 39f7ca16aa8b4654deb6a0bb790830941cd9f455.

• 91c9936f6a8f8a986aa5abc282cc1349e70caee4 rejects query/fragment relay-proxy bases and invalid explicit ports before SDK construction, while preserving valid root/path-prefix endpoints and sanitized errors. Public-constructor regression coverage includes query, fragment, :notaport, and :99999 cases.
• 39f7ca16aa8b4654deb6a0bb790830941cd9f455 routes the real compile-contract failure assertions through diagnostics that include the exact fixture-lock regeneration command for both valid-arm failures and invalid arms whose expected compile diagnostic is masked. A deliberately stale fixture lock produced the command in the actual failing test output.

Fresh Blox verification at this exact head:

• cargo test --locked -p buzz-feature-flags: 9 + 15 + 5 pass
• cargo test --locked -p buzz-feature-flags --all-features: 33 + 15 + 5 pass
• all-feature/all-target Clippy with -D warnings, formatting, locked relay check, and locked metadata pass
• root and fixture lockfiles are unchanged
• worktree clean

The requested public constructor → start → evaluate → close loopback rerun is being dispatched independently against this exact SHA; I’ll post that evidence separately.

AI-generated response by Elrond (Codex).

[TheSentinel454]

Legolas focused re-review at exact head 39f7ca16aa8b4654deb6a0bb790830941cd9f455, limited to 657e6279b0ae21a7460203c4cf05da7c8b8814e6..39f7ca16aa8b4654deb6a0bb790830941cd9f455: clean (0 P0 / 0 P1 / 0 P2).

The public constructor now rejects query, fragment, non-numeric-port, and out-of-range-port bases before SDK construction while retaining root, path-prefix, and IPv6 endpoints. I independently traced the pinned SDK 3.2.0 suffix composition (/all, /bulk) and mutation-checked each new query/fragment/port guard: removing any one makes the public-constructor regression fail.

The fixture-lock recovery recipe is now present on both real status-failure seams. I deliberately staled the checked-in fixture lock: the valid-arm failure exited 101 and printed the exact regeneration command. I also fed that same nested-Cargo stale-lock output through the invalid-arm production assertion seam; it exited 101 and printed the same command.

Fresh Hermit evidence at this SHA:

• locked default suite: 9 + 15 + 5 pass;
• locked all-feature suite: 33 + 15 + 5 pass;
• all-target/all-feature Clippy with -D warnings, package formatting, locked relay check, and locked metadata pass;
• git diff --check clean; worktree clean; live PR head matches.

Runtime constructor → start → stream → evaluate → analytics shutdown confirmation remains with the separately dispatched E2E lane; this source review found no additional issue.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Review clear: no blocking source defects found at 39f7ca16aa8b4654deb6a0bb790830941cd9f455 against base 312cf674fad5563c24a66ec5350d306d192ed64a.

The earlier endpoint-validation, exact-integer-default, test-egress, and fixture-lock diagnostic findings are addressed. The complete compile-time provider matrix matches the intended consumer-free crate boundary; production relay wiring remains a separate change. Two inline documentation suggestions are non-blocking.

Source-only review; no code or tests executed. Existing CI ran all 53 feature-flag tests successfully on merge 78dfa667 containing this head. The separate security review remains stale, and I did not find a posted exact-head result for the previously requested lifecycle loopback rerun. This review does not replace those checks or claim real-proxy/runtime validation.

[TheSentinel454]

Reviewed the force-rebased PR at exact head 421b1116ac6f1861deaee983ac8ee937317d5198 (tree d1e15fdfe333134f6c09d5a5cf612cdb0b835858) against base ea1e97e65f816a1f4d92f2da6e3498eb90f5d361.

No P0/P1/P2 findings.

• The pre-fix 18-commit series is patch-equivalent to the previously reviewed series under git range-diff, apart from expected root/fixture lock context regenerated against the new base. The PR remains limited to the intended 15 files.
• The final review-fix commit accurately documents the adapter's community and optional pubkey kinds, omitted contextKind defaulting to user, missing-kind bucket zero selecting the first positive-weight variation, and synchronous blocking close() semantics. I traced these claims through pinned launchdarkly-server-sdk-evaluation 2.2.1 and launchdarkly-server-sdk 3.2.0 source.
• The new production-evaluator weighted-rollout regression is discriminating: changing explicit Some("community") to omitted None made the focused test fail at launchdarkly.rs:602; restoring exact source made it pass. This satisfies the review criteria themes discriminating regression tests and evidence standards.
• Lock validation: both default and all-feature locked suites pass, the locked relay build and locked metadata pass, the fixture lock has the same package/version/source multiset as the previously reviewed fixture, and the rebased root lock removes no package/version/source tuple present on the new base. This satisfies evidence standards and scope discipline.

Fresh exact-head verification:

• cargo test --locked -p buzz-feature-flags: 9 + 15 + 5 = 29 pass.
• cargo test --locked -p buzz-feature-flags --all-features: 34 + 15 + 5 = 54 pass.
• all-target/all-feature Clippy with -D warnings, package formatting, locked relay build, locked metadata, git diff --check, and clean-worktree checks pass.

CI is not clean at review time: Relay-Driven Mesh Lifecycle Smoke failed before build/test execution because Docker returned unauthorized while pulling MinIO service images on all three retries. I classify that observed failure as an infrastructure/registry-auth failure, not a feature-flag code failure; I am not labeling it flaky. Eleven broader jobs remain pending.

Verdict: clean — 0 P0, 0 P1, 0 P2.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Corrective review clear: no new blocking defect at HEAD 421b1116ac6f1861deaee983ac8ee937317d5198, BASE ea1e97e65f816a1f4d92f2da6e3498eb90f5d361.

The rebased 18-commit series preserves the previously reviewed behavior; the new five-file correction is documentation/test-only. Both prior optional suggestions are addressed: named LaunchDarkly context-kind rollout configuration and blocking close() guidance. The new weighted-rollout test calls the production evaluator and distinguishes absent-kind bucketing from declared-default fallback. The consumer-free crate boundary remains unchanged; relay wiring and real-proxy lifecycle verification belong to the consumer integration.

Validation: source-only review on the pinned Blox; no checkout, build, tests, or runtime probes executed by this review. Existing exact-head Rust Unit Tests passed 54/54 feature-flag tests, including the exact-one provider compile matrix. The fixture lock retains the prior package/version/source set, and the rebased root lock removes none from its exact base.

Validation limits: overall CI is not green; broader lifecycle/integration failures are not attributed to this crate here. The separate security review is marked stale for this range. No post-fix public-constructor→start→evaluate→close loopback result was found in the PR evidence inspected. This bounded source review does not clear those limits or grant merge approval.

[TheSentinel454]

Focused rebase review at exact head b166f66c31a22f957f8dbfd5896c055738d51a91 (tree 0a1e2841a768cd73d868f2ffb173278e82b035b6; base 797012ff01a6d499959b45ed2e56f7927c6a4d6b).

No P0/P1/P2 findings.

• git range-diff ea1e97e6..421b1116 797012ff..b166f66c reports = for all 19 commits. Independent stable patch IDs match 19/19 in order. The base-to-head file list is unchanged at the same 15 intended feature-flag/workspace/test files.
• #7869 (99c2acf90c) and #7870 (797012ff01) are ancestors through the new base. #7869 adds the published GHCR MinIO image and CI Compose override; #7870 wires that override into mesh and integration jobs. No .github, Compose, Dockerfile, Helm, or MinIO file appears in this PR's base-to-head diff, so the remediation is inherited rather than duplicated into feature scope.
• All 19 rebased commits retain the expected author, DCO sign-off, and Codex co-author trailer.

Fresh exact-head Hermit controls: locked default 29/29; locked all-feature 54/54; all-target/all-feature Clippy with -D warnings, package formatting, locked relay build, locked metadata, diff check, exact HEAD/tree, and clean-worktree checks pass.

The replacement Relay-Driven Mesh Lifecycle Smoke has already cleared the formerly failing Start integration services step and is now executing the relay-driven smoke. Other CI jobs remain in progress, so this is not a claim that the full check set is terminal or green.

Criteria: evidence standards and scope discipline.

Verdict: clean — 0 P0, 0 P1, 0 P2.

[TheSentinel454]

@buzz-security-review b166f66

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Review clear: no blocking defects found at HEAD b166f66c31a22f957f8dbfd5896c055738d51a91, BASE 797012ff01a6d499959b45ed2e56f7927c6a4d6b. This is a review comment, not merge approval.

Independent SDK and build/CI lanes are clear. I verified all 19 patches are unchanged from the previously reviewed series, read the public/environment/adapter and compile-contract paths, and reconciled the prior findings. The crate remains consumer-free; exact-one enforcement is an executable future-artifact contract, not current relay wiring.

The refreshed security review describes real configuration risks, but does not establish a violation of this PR’s documented contract:

• Targeting: global pubkey targeting is explicit; community-specific targeting must include community conditions. Flags must never authorize cross-community access.
• Transport: HTTP requires an explicitly supplied proxy endpoint; it is not a downgrade of the default HTTPS endpoints. Deployments must protect the credential-bearing hop with HTTPS or equivalent network protection.
• Integers: SDK numbers are already f64. The documented contract accepts integral, in-range SDK values, not lossless arbitrary decimal integers. Nonrepresentable configured values can round; consumers must constrain their ranges. Declared i64 fallback defaults remain exact.

Validation: existing Rust Unit Tests passed all 54 feature-flag tests, including the eight-case compile matrix, on merge 212525ecd3fe641c0a1ae0dc20dc150c17256d43 whose parents are the exact base and head. Current check snapshot: 54 successful, 28 skipped, no failures. No local suite rerun.

Limits: no new runtime probe; the post-fix constructor→start→evaluate→close loopback rerun is still not evidenced in the PR material inspected. Real proxy/TLS, secrets and lifecycle integration remain obligations of the future consumer change, not claims made by this review.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Incremental review clear: no new blocking defects found at HEAD b166f66c31a22f957f8dbfd5896c055738d51a91, BASE 797012ff01a6d499959b45ed2e56f7927c6a4d6b. This is a comment, not approval. The existing exact-head review and separate approval are credited, not issued by this run.

• Independently verified the rebase preserves all 19 feature patches and all 15 final feature-file blobs. The five-commit base advance has no direct file overlap; manifest, lockfile and unit-test integration remain intact. Prior contract dispositions stand. This remains a consumer-free crate plus a future-artifact exact-one compile contract, not production relay wiring.
• Existing CI passed 54/54 feature-flag tests on synthetic merge 212525ecd3fe641c0a1ae0dc20dc150c17256d43, whose parents are the exact base and head. The passing compile-contract test contains three valid and five invalid selections, independently counted in pinned source; the CI log does not enumerate each internal selection.
• Source-only review on the pinned Blox host; no checkout, execution, test rerun or new runtime probe. The post-fix LaunchDarkly constructor → start → evaluate → close loopback rerun remains unevidenced; broad mesh smoke does not close that gap. Real proxy/TLS, secrets and consumer lifecycle integration remain future-consumer obligations, not claims of this review.