Skip to content

Integrate Krill Switch feature flags - #3285

Open
jesse-merhi wants to merge 2 commits into
mainfrom
jesse/integrate-krill-switch-flags
Open

Integrate Krill Switch feature flags#3285
jesse-merhi wants to merge 2 commits into
mainfrom
jesse/integrate-krill-switch-flags

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Jul 28, 2026

Copy link
Copy Markdown
Member

Summary

  • Add the official @openclaw/krillswitch-react SDK with a single typed souls proof flag.
  • Evaluate flags in the root loader during SSR with a 200 ms fail-open budget.
  • Reuse the server value during hydration so the homepage copy does not flash from the default state.
  • Keep anonymous targeting stable with a first-party HTTP-only cookie, then refresh values in the browser through the SDK.
  • Document that flags are progressive delivery controls, never an authorization boundary.

When souls is enabled, the homepage subtitle becomes “Discover skills and plugins built with soul.” When disabled or unavailable, it remains “Discover skills and plugins from top creators.”

Dependency prerequisite

Merge Krill Switch #43 and publish @openclaw/krillswitch-react@0.0.1 before merging this PR. The lockfile entries were prepared from the package artifacts at Krill commit 51df4d3; regenerate the lockfile if the published artifacts differ.

Local proof

  • Raw SSR HTML contained the enabled souls copy.
  • With client evaluation delayed by 1.5 seconds, the browser never rendered the fallback copy at 390, 768, or 1440 px widths.
  • Missing configuration and evaluation failures preserve the code-owned default.

Checks

  • bun run test --run src/lib/featureFlags.test.tsx src/lib/featureFlags.server.test.ts — 4 tests passed
  • npm_config_registry=https://registry.npmjs.org bun run ci:static — passed
  • bun run ci:unit — 5,610 tests passed
  • bun run ci:types-build — passed
  • bun run build — passed

Automated autoreview was attempted twice but its isolated reviewer could not authenticate to the OpenAI Responses endpoint (401 Unauthorized). A manual review of the complete PR diff and installed SDK contract found no actionable issues.

Copilot AI review requested due to automatic review settings July 28, 2026 06:55
@jesse-merhi
jesse-merhi requested a review from a team as a code owner July 28, 2026 06:55
@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Error Error Jul 28, 2026 3:26pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Aug 5, 2026
@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 5, 2026, 3:28 PM ET / 19:28 UTC.

ClawSweeper review

What this changes

The PR adds Krill Switch SDK evaluation during server rendering and hydration so a typed flag can vary the homepage subtitle without a default-state flash.

Merge readiness

Blocked by patch quality or review findings - 11 items remain

Keep this member-authored PR open: current main still uses the fixed homepage subtitle, while the unchanged branch retains blockers around pre-publication dependency integrity, root-wide evaluation, and missing inspectable browser proof.

Priority: P2
Reviewed head: 847a6435455187e5db6ef73b4524e6dddcb86cb2
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🧂 unranked krab (1/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 4 items Current main remains unflagged: Current main renders the fixed homepage subtitle and has no Krill Switch feature-flag implementation, so the central behavior has not already landed.
Published artifact integrity remains unverified: The PR body says the Bun lock entries were prepared from pre-publication artifacts and must be regenerated if the package differs after publish; the unchanged lockfile pins both SDK packages with those hashes.
Global evaluation remains in the root loader: The PR attaches the server function to the root route rather than the only current consumer, the homepage, so every route can incur the 200 ms external evaluation budget and global provider polling.
Findings 3 actionable findings [P1] Regenerate SDK lock entries from published artifacts
[P2] Scope initial evaluation to the homepage
[P3] Attach inspectable browser proof for the hero variant
Security Needs attention Verify published SDK bytes before installing them: The declared dependency has pre-publication integrity hashes, so the reviewed supply-chain artifact is not yet the one Bun will resolve from the registry.

How this fits together

The TanStack Start root route renders every ClawHub page and wraps the application’s global providers. This PR obtains an anonymous flag context at that boundary, calls the flag service, then feeds the hydrated value to the homepage hero.

flowchart LR
  A[Visitor request] --> B[Root route loader]
  B --> C[Anonymous context cookie]
  C --> D[Flag evaluation service]
  D --> E[Hydrated flag provider]
  E --> F[Homepage hero subtitle]
Loading

Decision needed

Question Recommendation
Should ClawHub adopt Krill Switch as a global runtime feature-flag dependency for a copy-only homepage experiment? Approve a homepage-scoped trial: Adopt the SDK only around the homepage proof flag after publishing the package, regenerating the lockfile, and supplying real-browser evidence.

Why: The patch introduces a new external SDK, environment contract, root-route network dependency, anonymous targeting cookie, and polling behavior; choosing the permanent scope is a product and platform decision beyond mechanical review.

Before merge

  • Regenerate SDK lock entries from published artifacts (P1) - The PR explicitly says these integrity hashes came from pre-publication artifacts. Bun treats them as the package-byte contract, so installing published 0.0.1 can either fail or use a different SDK from the reviewed one. Publish first, inspect the registry package, and regenerate this lock section. This remains the earlier P1 finding on the unchanged head.
  • Scope initial evaluation to the homepage (P2) - The only consumer is the homepage subtitle, but this root-route loader runs the external 200 ms evaluation budget and globally mounts the polling provider for every page. Move the loader/provider into the homepage path so unrelated routes do not inherit the dependency and latency. This remains the earlier P2 finding on the unchanged head.
  • Attach inspectable browser proof for the hero variant (P3) - The PR describes local checks but provides no screenshot, recording, browser output, or linked redacted artifact for the changed copy. Add after-fix real-browser proof that shows enabled SSR output and hydration without a fallback flash; redact private endpoints and identifiers. This remains the earlier P3 finding on the unchanged head.
  • Resolve security concern: Verify published SDK bytes before installing them - The declared dependency has pre-publication integrity hashes, so the reviewed supply-chain artifact is not yet the one Bun will resolve from the registry.
  • Resolve merge risk (P1) - The pre-publication Bun integrity hashes can make installs fail or execute bytes different from the reviewed SDK once version 0.0.1 is published.
  • Resolve merge risk (P1) - A 200 ms external evaluation and browser polling are attached to the root application boundary although only the homepage reads the flag.
  • Resolve merge risk (P1) - No linked screenshot, recording, browser output, or redacted live artifact proves the claimed SSR/hydration behavior.
  • Complete next step (P2) - Maintainer product direction is required for the global-versus-homepage feature-flag boundary; contributor proof and published-artifact verification remain merge blockers.
  • Improve patch quality - Resolve the security review concern or explain why the changed path is safe.
  • Improve patch quality - Address the highest-priority review finding and re-run the changed-surface validation.

Findings

  • [P1] Regenerate SDK lock entries from published artifacts — bun.lock:456-458
  • [P2] Scope initial evaluation to the homepage — src/routes/__root.tsx:33-34
  • [P3] Attach inspectable browser proof for the hero variant — src/routes/index.tsx:40-44
  • [medium] Verify published SDK bytes before installing them — bun.lock:456
Agent review details

Security

Needs attention: The branch introduces an external server/browser SDK and anonymous targeting identifier, but the lockfile hashes are knowingly not tied to a published artifact.

Review metrics

Metric Value Why it matters
Change surface +325/-13 across 13 files A copy-only experiment introduces a dependency, root runtime work, cookie handling, hydration state, tests, and operational documentation.
Root-route budget 200 ms per initial root evaluation The externally bounded work is placed at the shared application entry point rather than its sole homepage consumer.
Production versus test growth production +159/-13, tests +126, specs +40 The runtime footprint is much larger than the one current hero-copy consumer, reinforcing the need to settle the scope.

Merge-risk options

Maintainer options:

  1. Repair and scope the trial (recommended)
    Regenerate dependencies from published artifacts, move flag evaluation/provider use to the homepage, and attach redacted real-browser SSR/hydration proof before merge.
  2. Accept global delivery infrastructure
    Explicitly accept the root-wide external call, anonymous identifier, and polling contract after verifying the published SDK and production operational budget.
  3. Pause the experiment
    Do not merge the dependency until product owners choose a durable feature-flag platform scope.

Technical review

Best possible solution:

Publish and independently inspect the SDK, regenerate its lock entries from the registry artifact, then restrict initial evaluation and the provider to the homepage before attaching real-browser SSR/hydration proof.

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

Yes for the route-scope and package-integrity blockers: the source shows root-level evaluation and pre-publication lock hashes, though this read-only review did not run the branch or install the unpublished package.

Is this the best way to solve the issue?

No; a homepage-only provider and loader are the narrower maintainable path for the sole souls copy consumer, after published-package verification.

Full review comments:

  • [P1] Regenerate SDK lock entries from published artifacts — bun.lock:456-458
    The PR explicitly says these integrity hashes came from pre-publication artifacts. Bun treats them as the package-byte contract, so installing published 0.0.1 can either fail or use a different SDK from the reviewed one. Publish first, inspect the registry package, and regenerate this lock section. This remains the earlier P1 finding on the unchanged head.
    Confidence: 0.98
  • [P2] Scope initial evaluation to the homepage — src/routes/__root.tsx:33-34
    The only consumer is the homepage subtitle, but this root-route loader runs the external 200 ms evaluation budget and globally mounts the polling provider for every page. Move the loader/provider into the homepage path so unrelated routes do not inherit the dependency and latency. This remains the earlier P2 finding on the unchanged head.
    Confidence: 0.96
  • [P3] Attach inspectable browser proof for the hero variant — src/routes/index.tsx:40-44
    The PR describes local checks but provides no screenshot, recording, browser output, or linked redacted artifact for the changed copy. Add after-fix real-browser proof that shows enabled SSR output and hydration without a fallback flash; redact private endpoints and identifiers. This remains the earlier P3 finding on the unchanged head.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 109384dcb8f0.

Labels

Label justifications:

  • P2: This is a bounded homepage experiment, but its current root-level dependency and install blocker need normal-priority maintainer attention.
  • merge-risk: 🚨 compatibility: The branch adds a new published SDK and lockfile contract whose pre-publication hashes may not match the release artifact.
  • merge-risk: 🚨 security-boundary: A new external SDK runs in server and browser paths and receives a stable anonymous targeting identifier, but the exact published bytes have not been verified.
  • merge-risk: 🚨 availability: The root loader can invoke the remote evaluation service with a 200 ms timeout for routes that do not consume the flag.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

Security concerns:

  • [medium] Verify published SDK bytes before installing them — bun.lock:456
    The declared dependency has pre-publication integrity hashes, so the reviewed supply-chain artifact is not yet the one Bun will resolve from the registry.
    Confidence: 0.98

What I checked:

  • Current main remains unflagged: Current main renders the fixed homepage subtitle and has no Krill Switch feature-flag implementation, so the central behavior has not already landed. (src/routes/index.tsx:38, 87ca030c30f3)
  • Published artifact integrity remains unverified: The PR body says the Bun lock entries were prepared from pre-publication artifacts and must be regenerated if the package differs after publish; the unchanged lockfile pins both SDK packages with those hashes. (bun.lock:456, 847a64354551)
  • Global evaluation remains in the root loader: The PR attaches the server function to the root route rather than the only current consumer, the homepage, so every route can incur the 200 ms external evaluation budget and global provider polling. (src/routes/__root.tsx:34, 847a64354551)
  • Branch unchanged since prior review: The current head is the same SHA reviewed in the prior ClawSweeper cycle, so its three findings remain unresolved rather than newly introduced. (src/lib/featureFlags.functions.ts:48, 847a64354551)

Likely related people:

  • Patrick Erichsen: The current root document, application providers, and homepage implementation all trace to the current release commit, which is the baseline this PR changes. (role: recent main-path contributor; confidence: medium; commits: 87ca030c30f3; files: src/components/AppProviders.tsx, src/routes/__root.tsx, src/routes/index.tsx)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (15 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-05T09:42:28.643Z sha 847a643 :: found issues before merge. :: [P2] Scope flag evaluation to the homepage route | [P3] Attach real-browser proof for the flagged homepage copy
  • reviewed 2026-08-05T11:11:08.529Z sha 847a643 :: found issues before merge. :: [P2] Scope flag evaluation to the homepage route | [P1] Regenerate the lockfile after the SDK is published | [P3] Attach inspectable browser proof for the flagged copy
  • reviewed 2026-08-05T11:20:56.982Z sha 847a643 :: found issues before merge. :: [P1] Regenerate the SDK lock entries from the published package | [P2] Scope SSR flag evaluation to the homepage route | [P3] Attach inspectable real-browser proof for the flagged copy
  • reviewed 2026-08-05T12:31:02.265Z sha 847a643 :: found issues before merge. :: [P1] Regenerate the SDK lock entries from the published package | [P2] Move initial flag evaluation out of the root route
  • reviewed 2026-08-05T14:04:34.626Z sha 847a643 :: found issues before merge. :: [P1] Regenerate SDK lock entries from the published package | [P2] Scope initial flag evaluation to the homepage route | [P3] Attach inspectable real-browser proof for the hero variant
  • reviewed 2026-08-05T16:05:13.994Z sha 847a643 :: found issues before merge. :: [P1] Regenerate SDK lock entries from published artifacts | [P2] Scope initial flag evaluation to the homepage | [P3] Attach inspectable real-browser proof
  • reviewed 2026-08-05T16:16:34.977Z sha 847a643 :: found issues before merge. :: [P1] Regenerate SDK lock entries from published artifacts | [P2] Scope initial flag evaluation to the homepage | [P3] Attach inspectable browser proof for the hero variant
  • reviewed 2026-08-05T18:23:08.890Z sha 847a643 :: found issues before merge. :: [P1] Regenerate SDK lock entries from published artifacts | [P2] Scope initial flag evaluation to the homepage | [P3] Attach inspectable browser proof for the hero variant

@Patrick-Erichsen Patrick-Erichsen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces Krill Switch as a remote feature-flag provider and uses it for homepage hero copy. It is not merge-ready because it adds a new production/SSR dependency before the package contract and operating model are established.

LOC: +325/-13 (13 files)

Findings: rebase the conflicted branch, verify the lock entry against the published SDK tarball, add real-browser enabled/default/fail-open proof, and get an explicit product decision to reintroduce remote flag machinery after the prior flag system was removed.

Best-fix verdict: too broad for one copy experiment. A platform dependency is justified only if ClawHub is committing to a supported flag operating model beyond this single use.

Alternatives considered: a local config/build-time switch is simpler for one experiment; adopting the remote SDK can be reasonable once ownership, failure mode, privacy, and rollout policy are durable.

Code read: root provider/SSR wiring, homepage flag consumer, package/lock changes, tests, and current main’s lack of a flag platform.

Remaining uncertainty: published package identity/integrity, operational sponsorship, and browser behavior.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 5, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Aug 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale due to inactivity.
Please update it or it will be closed.

@github-actions github-actions Bot added the stale label Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. stale status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants