[roboclaw-bot]

Problem / impact

An unresolved variation in degraded stored configuration throws during evaluation, turning OFREP bulk requests into a global 500 and losing healthy flags. Admin validation normally prevents these references; this is corruption/degraded-data isolation, not a demonstrated normal-write bug.

Solution

• Return sanitized keyed GENERAL failures per flag (single HTTP 400, mixed bulk HTTP 200), keeping healthy results and existing ETag revalidation/recovery. The failure message directs callers to ask the flag owner to check its configuration and retry after repair.
• Leave native /v1/eval, evaluator ordering, targeting/rollout assignments, disabled precedence, and reason mappings untouched.
• Document official server/web provider setup, origin URL, bearer environment eval keys, flat targeting context, typed defaults and per-flag errors. Both provider examples use the required header-pair arrays.

Evidence

• Real Worker/local-D1 regressions: 4 failures on base (500 instead of mixed 200); 17 OFREP tests pass with the repair, including default/off/target corruption, sanitization, ETag/304 and corrected-config recovery after TTL.
• Full lint, typecheck, tests (147 Worker tests plus core/React/CLI/script suites), docs build and SDK package verification pass on the initial implementation.
• Review follow-up eecdac3: reproduced the object-header spread TypeError, verified header-pair construction, and reran all 17 OFREP tests, lint, full workspace typecheck, docs build and git diff --check successfully.
• Required P0–P3 autoreview: scoped-clean on the implementation and review follow-up; no actionable findings. Both hosted findings are addressed in eecdac3.

Independent follow-up to #181, based on current main; it does not include or depend on that PR's changes. No schema/auth/dependency/SDK/release/deployment changes.

Worked on by

• @vincentkoc

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs changes before merge.

What this changes

This PR returns sanitized per-flag OFREP failures while preserving healthy bulk results and documents official OpenFeature provider setup.

Example: The souls flag references missing-variation-internal-detail while theme remains valid.

• Before: Bulk evaluation returns HTTP 500 and loses the healthy theme result.
• After: Bulk evaluation returns HTTP 200 with a keyed GENERAL failure for souls and the healthy theme result.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Useful, focused isolation with meaningful Worker coverage, held below merge readiness by the provider setup defect.
Proof confidence	🦐 gold shrimp (3/6)	Under the supplied maintainer-authored classification, the ordinary contributor proof gate does not apply; available evidence exercises the real Worker and local D1 through tests, without a standalone provider trace.
Patch quality	🦐 gold shrimp (3/6)	2 actionable review findings remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: One corrupt flag prevents OpenFeature clients from receiving healthy flags in the same environment.
Reason: Protocol-compatible isolation has clear value, preserves valid behavior, and has owner direction under the supplied maintainer-authored classification.

Merge readiness

⛔ Needs changes before merge - 2 items remain

This PR addresses a real degraded-configuration failure still present on main, but the new provider examples contain a setup-breaking header format error.

Priority: P2
Reviewed head: f4d4fb99563a1beca7f7f78cc24d90da637c1c50

Before merge

• Use header-pair arrays in both official provider examples (P2) - Both this server example and the browser example at line 115 pass headers as an object. The official shared provider options require [string, string][] and buildHeaders spreads options.headers into an array. Copying either example therefore produces a TypeScript error or throws before sending an evaluation request. Use headers: [["Authorization", Bearer ${evalKey}]] and the equivalent browser form.
• Include a recovery action in the sanitized failure message (P3) - When degraded configuration triggers this branch, official providers expose this text as errorMessage. "Flag evaluation failed." gives callers no next action and does not distinguish a configuration problem from a transient request failure. Keep internal identifiers hidden while directing the caller to ask the flag owner to check its configuration and retry after repair; update the documented response and assertions consistently.

Findings

• [P2] Use header-pair arrays in both official provider examples — docs/api.md:92
• [P3] Include a recovery action in the sanitized failure message — apps/api/src/ofrep.ts:75

Tests

• Missing end-to-end proof: Supplied validation covers Worker/local-D1 regressions, without a standalone official-provider-to-Worker trace demonstrating mixed-result defaults and recovery.

Agent review details

How this fits together

The public Worker's OFREP router resolves environment configuration through the D1-backed cache, invokes the shared evaluator, and returns OpenFeature-compatible results.

flowchart LR
  A[OpenFeature provider] --> B[Public Worker boundary]
  B --> C[OFREP router]
  C --> D[Configuration cache]
  D --> E[D1 database]
  C --> F[Core evaluator]
  F --> G[Per flag result or failure]
  G --> A

Loading

Technical review

Best possible solution:

Keep the OFREP-local isolation, correct both examples to use header pairs, and provide a sanitized recovery action in the failure message.

Do we have a high-confidence way to reproduce the issue?

Source establishes the failure deterministically: a missing variation throws from the core evaluator and escapes main's bulk mapping; added Worker/D1 cases exercise that input, but no target code was executed during this review.

Is this the best way to solve the issue?

The OFREP adapter is the right owner for protocol-specific failure isolation because changing the shared evaluator would alter native and SDK contracts; the documentation must match the verified provider option types.

Full review comments:

• [P2] Use header-pair arrays in both official provider examples — docs/api.md:92
  Both this server example and the browser example at line 115 pass headers as an object. The official shared provider options require [string, string][] and buildHeaders spreads options.headers into an array. Copying either example therefore produces a TypeScript error or throws before sending an evaluation request. Use headers: [["Authorization", Bearer ${evalKey}]] and the equivalent browser form.
  Confidence: 1
• [P3] Include a recovery action in the sanitized failure message — apps/api/src/ofrep.ts:75
  When degraded configuration triggers this branch, official providers expose this text as errorMessage. "Flag evaluation failed." gives callers no next action and does not distinguish a configuration problem from a transient request failure. Keep internal identifiers hidden while directing the caller to ask the flag owner to check its configuration and retry after repair; update the documented response and assertions consistently.
  Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 666708d2ebd8.

Provenance checked

• OFREP evaluation and response ownership keeps the original intent (ebf9a0e: feat(api): OpenFeature Remote Evaluation Protocol (OFREP) endpoints #53 added vendor-neutral evaluation through the existing evaluator, cache, and shared helpers.)
• Public evaluation hostname documentation keeps the original intent (9786b22: docs: add KrillSwitch documentation site #20 documented the original native-only public endpoint; current routing also supports OFREP and streaming.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

Acceptance criteria:

• Check both provider snippets against the official OFREPProviderBaseOptions.headers type.
• pnpm --dir apps/api exec vitest run test/ofrep.test.ts
• pnpm lint
• pnpm typecheck
• pnpm docs:build

What I checked:

• Failure remains on main and latest release: Both main and v0.6.1 call evaluateFlag directly inside bulk mapping; an unresolved variation throws before the response can preserve healthy flags. (apps/api/src/ofrep.ts:142, fa559f4be47b)
• Focused production repair: The introduced diff catches individual evaluator failures in the OFREP adapter, preserves keyed results, and retains existing authentication, ETag handling, and shared evaluation semantics. (apps/api/src/ofrep.ts:62, f4d4fb99563a)
• Authoritative provider header contract: The new documentation explicitly claims compatibility with these providers. OFREPProviderBaseOptions.headers requires [string, string][]; buildHeaders spreads that array, so the object supplied by both examples fails before network I/O. (libs/shared/ofrep-core/src/lib/provider/ofrep-provider-options.ts, bc2ef4865454)
• Per-flag fallback compatibility: The server provider accepts keyed evaluation failures on error statuses; the web provider caches individual responses and resolves keyed failures through handleEvaluationError, which returns the application default. (libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts, bc2ef4865454)
• Original design and ownership: feat(api): OpenFeature Remote Evaluation Protocol (OFREP) endpoints #53 introduced OFREP as an adapter over the existing evaluator and shared helpers. Feature history identifies Vincent Koc across implementation and tests; a missing creation-parent blob limits follow-history traversal but the creation diff is available. (apps/api/src/ofrep.ts:29, ebf9a0e2bea2)
• Distinct adjacent work: fix(ofrep): preserve provider-readable context errors #181 repairs request-context validation and error envelopes, whereas this PR isolates evaluator failures; neither replaces the other's central repair. (0fbe961d2219)

Likely related people:

• Vincent Koc: Raw commit ebf9a0e adds apps/api/src/ofrep.ts:29 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: ebf9a0e2bea2; files: apps/api/src/ofrep.ts)
• Peter Steinberger: Raw commit 9786b22 adds docs/api.md:6 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 9786b2291102; files: docs/api.md)

Review metrics

Metric	Value	Why it matters
Change footprint	Production +29/-5; tests +161; documentation +83/-1; changelog +4	Production growth stays within the existing protocol adapter; tests cover corruption isolation, sanitization, precedence, and cache recovery.

Labels

Label changes:

• add P2: Degraded configuration can discard healthy OFREP results, and the new setup examples prevent provider requests; neither establishes an urgent normal-write regression.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

• P2: Degraded configuration can discard healthy OFREP results, and the new setup examples prevent provider requests; neither establishes an urgent normal-write regression.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed November 1, 2026, 8:02 PM ET / November 2, 2026, 01:02 UTC.