Skip to content

feat(desktop): add protected-build Bestie experiment - #6902

Open
mahanti wants to merge 11 commits into
mainfrom
am-bestie-01-experiment
Open

feat(desktop): add protected-build Bestie experiment#6902
mahanti wants to merge 11 commits into
mainfrom
am-bestie-01-experiment

Conversation

@mahanti

@mahanti mahanti commented Aug 26, 2026

Copy link
Copy Markdown

Summary

Introduces a protected-build boundary for the default-off Bestie experiment without adding any Bestie product surface.

  • Official OSS builds select an empty protected-feature module and emit no Bestie/Chief metadata or implementation content.
  • Protected internal builds select a separate module graph containing the Bestie experiment definition.
  • Within an internal build, Bestie remains disabled until the user opts in under Settings → Experiments.
  • The production build runs an artifact matrix and fails if OSS output contains protected content or internal output lacks the Bestie manifest.

Build contract

Build variant User opt-in Result
Official OSS Any/forged Bestie absent from the compiled artifact
Protected internal Off Bestie available but disabled
Protected internal On Bestie enabled

The companion protected-release change is squareup/buzz-releases#91. It sets VITE_BUZZ_BESTIE=1, requires that exact value, forwards it into the signed macOS build, and asserts the contract in release validation.

Why this is separate

This gives later Bestie PRs one build-selected import seam. Protected implementations must be reachable only from the internal module so they never enter the official OSS module graph.

Non-goals

  • No Bestie persona or provisioning
  • No sidebar, app-chrome, or message-toolbar UI
  • No entitlement or secrecy claim: the source is public; this boundary controls official Block artifacts

Verification

  • Exact commit 523cf49ced03cba9be43836a54d6aa5d6923cc82
  • Full just ci: 5,673 Desktop tests, 2,773 Tauri tests, 1,860 mobile tests, Rust/Tauri/web/mobile static checks and builds
  • OSS production artifact: scanner confirms no Bestie, Chief of Staff, or builtin:bestie content
  • Internal production artifact: scanner confirms the protected Bestie manifest is emitted
  • Both build orders verified; dist retains the requested variant for Vite/Tauri packaging

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Reviewed eb8e97cfe74afbe560d92f3a8f470967a992767f..48892e446556df2ef53d55d4cc6831263714a3e7 at the exact live head. Risk is medium: this introduces a shared compile-time/runtime availability boundary intended to gate every later Bestie surface and side effect.

Blocking finding

[P2] The exported general resolver still fails open when build availability is omitteddesktop/src/shared/features/resolveEnabled.ts:13-20, re-exported by desktop/src/shared/features/useFeatureEnabled.ts and desktop/src/shared/features/index.ts.

The new buildAvailable parameter defaults to true, so the exported API introduced as part of this boundary returns:

resolveEnabled("bestie", { bestie: true })                 => true
resolveEnabled("bestie", { bestie: true }, false, false)   => false

This omission pattern already exists in production: desktop/src/features/settings/ui/SettingsView.tsx:138-142 calls resolveEnabled(s.featureGate, featureState) without build availability. Bestie does not currently define a Settings section, so today's Bestie toggle and hook path remain correctly fenced. The defect is in the PR's claimed reusable contract: a later UI, listener, query, or side-effect consumer can use the exported resolver in the same obvious way and silently bypass the build capability. The added truth table does not detect that path because every Bestie row explicitly supplies the fourth argument.

Author action: make build capability non-optional in exported/general resolution paths. Prefer a resolver that accepts the FeatureDefinition and derives isFeatureAvailableInThisBuild(feature) internally, then use it from both useFeatureEnabled and SettingsView; alternatively keep the low-level Boolean helper private and expose only a capability-safe resolver. Add a regression through the production entry point proving capability-off plus forged {bestie: true} resolves false. Add checked-in build-on/off coverage for experiment-toggle presence so the advertised boundary remains protected.

Verification owner: author for the code and regression; reviewer/CI for the refreshed exact head.

Verified behavior and validation

At exact clean head 48892e446556df2ef53d55d4cc6831263714a3e7:

  • Full Desktop unit suite passed: 5,676/5,676.
  • Desktop typecheck passed; Desktop check passed with five unrelated pre-existing diagnostics.
  • Production Vite builds passed with VITE_BUZZ_BESTIE absent and with VITE_BUZZ_BESTIE=1; coercion correctly accepts only literal "1".
  • Reviewer-only compiled E2E passed for capability-off forged storage, capability-on default/off, previous opt-in, toggle persistence, and accessible switch naming. Temporary test files were removed and the tree restored clean.
  • Capability-off Settings filtering and the useFeatureEnabled path correctly fail closed today. A full-repository bestie search found no Bestie product component, route, listener, query, native call, or network/process side effect at this head.
  • Local executable reproduction confirmed the omitted-capability call returns true while explicit capability-off returns false.
  • Current selected CI is green except Desktop Core, which remains pending. That is an external confidence gap, not separate author action unless it fails.

Residual risk

No signed internal release artifact or native WebView was inspected. Both compiled build variants were exercised, and release-pipeline injection is explicitly out of scope, so release artifact/pipeline audit remains with the release owner rather than the PR author. The current visible fence works; the reusable fence has a side door.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti

mahanti commented Aug 26, 2026

Copy link
Copy Markdown
Author

🤖 @buzz-security-review 2acd085

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: APPROVE

Reviewed eb8e97cfe74afbe560d92f3a8f470967a992767f..2acd085729d5d6f3c644fcfd13412dd2b95e48f8 at the exact live head. Risk is medium: this establishes a shared build-capability boundary that later Bestie surfaces and side effects are expected to trust.

The replacement head closes the prior blocker. The public barrel now exposes the build-derived resolveFeatureEnabledInThisBuild rather than the low-level resolver (desktop/src/shared/features/index.ts:1-19); both production general paths use it (desktop/src/shared/features/useFeatureEnabled.ts:92-105, desktop/src/features/settings/ui/SettingsView.tsx:133-145). The low-level helper requires a FeatureDefinition and mandatory capability Boolean and accepts only literal true (desktop/src/shared/features/resolveEnabled.ts:16-25), so JavaScript omission and forged persisted {bestie:true} fail closed in capability-off builds. Capability itself accepts only exact VITE_BUZZ_BESTIE === "1" (desktop/src/shared/features/runtimeBuildAvailability.ts:10-29).

No unresolved author-actionable defect was found. Searches across the reviewed Desktop checkout plus preview-features.json and .env.example found Bestie confined to manifest/build-capability wiring and tests—no Bestie route, component, listener, query, Tauri command, network/process effect, or other product consumer at this head.

Validation at exact clean head

  • Full Desktop unit suite: 5,678/5,678 passed.
  • just desktop-typecheck: passed.
  • just desktop-check: passed with five unrelated existing diagnostics.
  • Production builds with the capability unset and with VITE_BUZZ_BESTIE=1: passed.
  • Causal mutation restoring fail-open omission behavior failed the new omission regression; restored source passed the full suite.
  • Reviewer-only compiled E2E: capability-off forged/cross-build persistence hid Bestie while preserving another experiment as a positive control; capability-on default-off and prior-opt-in states rendered with accessible name Bestie, toggled, and persisted correctly. Temporary test files were removed.
  • Current CI: Desktop Core, Desktop Smoke shards 1–2, unit, integration, relay, security, platform build, and release-candidate checks passed. Smoke shards 3–4 were still pending at submission. Mark Previous Review Stale failed in review reconciliation; that is administrative stale-review handling rather than a product gate failure, and this approval supersedes the prior requested-changes review.
  • Final preflight: live/local head 2acd085729d5d6f3c644fcfd13412dd2b95e48f8, merge base eb8e97cfe74afbe560d92f3a8f470967a992767f, 13 files +205/-23, clean tree.

Author action: none.

Verification owner: CI for the two pending smoke shards; release owner for any signed-artifact audit.

Residual risk: the UI/cross-build E2E matrix is reviewer evidence rather than checked-in coverage. No signed internal release artifact or native WebView was inspected. The causal unit regression, both compiled build variants, and the compiled UI matrix provide proportionate evidence for this bounded change; release-pipeline injection remains outside this PR's stated scope.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:bot: Jude’s code review agent

APPROVE at exact head 2acd085729d5d6f3c644fcfd13412dd2b95e48f8 against base eb8e97cfe74afbe560d92f3a8f470967a992767f.

The prior fail-open resolver defect is fixed. The exported canonical path now derives capability from the running build, the low-level resolver requires explicit build availability and fails closed on JavaScript omission, and both current production consumers use the canonical resolver (desktop/src/shared/features/runtimeBuildAvailability.ts:10-29, useFeatureEnabled.ts:92-105, desktop/src/features/settings/ui/SettingsView.tsx:133-145). The package barrel no longer exports the low-level resolver. Forged persisted {bestie:true} cannot enable Bestie or expose its toggle in a capability-off build.

Author action: none.
Verification owner: release owner for any signed internal artifact/pipeline audit; CI for remaining exact-head jobs.

Integrated exact-head evidence:

  • Desktop unit suite: 5,678/5,678 passed.
  • Desktop typecheck and check passed; diagnostics were pre-existing and outside the PR files.
  • Production/Desktop builds passed with VITE_BUZZ_BESTIE unset and with =1.
  • Reviewer compiled E2E passed capability-off forged/cross-build persistence, capability-on default/opt-in/toggle persistence, and accessible switch naming.
  • Causal mutation restoring the prior fail-open behavior made the new omission regression fail; shipped source restored and full suite passed.
  • Scoped consumer search found no Bestie product surface, listener, query, native command, or process/network side effect beyond manifest/capability/test wiring.

Residual risk: the compiled Settings cross-build matrix is reviewer evidence rather than checked-in coverage, and no signed internal release artifact/native WebView was inspected. Those are confidence gaps, not author defects. Any new head invalidates this approval.

@wesbillman wesbillman 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.

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

Verdict: REQUEST CHANGES

Reviewed exact head 2acd085729d5d6f3c644fcfd13412dd2b95e48f8 against base eb8e97cfe74afbe560d92f3a8f470967a992767f for the required boundary: Chief/Bestie is available in protected buzz-releases builds and absent from OSS builds.

Blocking finding

[P2] This gates runtime visibility, but does not exclude Chief/Bestie from OSS artifactspreview-features.json:35-40, statically imported by desktop/src/shared/features/manifest.ts:1-56.

I built this head twice with the same production command: once with VITE_BUZZ_BESTIE unset and once with VITE_BUZZ_BESTIE=1. The unset build correctly bakes the capability off, but its emitted main JavaScript bundle still contains the complete Bestie manifest entry (id, name, description, requiredBuildFlag) and the Bestie build-gating schema/logic. This is therefore an availability boundary, not an artifact-exclusion boundary. Later statically imported Chief/Bestie modules are likewise not guaranteed to be absent merely because rendered consumers resolve false.

There is also no end-to-end internal variant yet: current squareup/buzz-releases@bb445f4a76045697dd11919985887fb4effcaf7d neither defines nor explicitly forwards VITE_BUZZ_BESTIE, so its standard signed macOS build takes the same capability-off path as OSS. The release wiring can remain a companion change, but it must land before the internal-enabled behavior is claimed.

Author action: make the build select an OSS manifest/module graph that contains no Chief/Bestie entry or implementation, rather than statically importing those assets and hiding them at runtime. Add an artifact-level regression that builds both variants and proves (1) OSS output has no Chief/Bestie manifest or implementation content and (2) the internal variant includes it. Land the protected buzz-releases flag forwarding plus a release-contract assertion as the integration companion. If the intended requirement is only “official OSS UI hidden/disabled,” that is a materially weaker product boundary and should be confirmed with Wes before replacing artifact exclusion.

What is working

  • Only the exact compile-time string VITE_BUZZ_BESTIE=1 enables availability; unset, 0, true, whitespace, and runtime mutation fail closed.
  • The experiment picker filters unavailable features, canonical feature resolution includes build availability, and forged persisted {bestie:true} cannot enable or expose Bestie in a capability-off build.
  • A scoped search of this head found no current Bestie product component, listener, query, IPC/native command, or side effect beyond manifest/gate wiring.
  • The low-level resolver requires explicit build availability and fails closed for JavaScript omission.

Validation and residual risk

  • Both production Vite builds completed at the exact clean head. The OSS/unset bundle directly reproduced the leaked manifest metadata; git diff --check was clean.
  • Existing exact-head CI covers the new Node unit tests. The runtime import.meta.env decision and artifact-content matrix are not checked in. One Desktop smoke shard currently fails in unrelated workflow/video tests; this PR does not touch those paths, so I am not treating that as this change’s defect.
  • This public source flag cannot be an entitlement or security boundary: any downstream source builder can set it. Artifact exclusion can protect official OSS distribution contents, not prevent custom builds.

Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
@mahanti mahanti changed the title feat(desktop): add build-gated Bestie experiment feat(desktop): add protected-build Bestie experiment Aug 27, 2026
@mahanti

mahanti commented Aug 27, 2026

Copy link
Copy Markdown
Author

🤖 Responding on Arjun’s behalf: we confirmed that the intended boundary is the stronger artifact-exclusion model—Bestie should exist only in the protected internal build, not merely be hidden in OSS.

Carl’s requested changes are addressed at exact head 523cf49ced03cba9be43836a54d6aa5d6923cc82:

  • preview-features.json is OSS-only and contains no Bestie entry.
  • Vite selects either an empty public protected-feature module or the internal Bestie module at build time, so the OSS graph never imports protected metadata/implementation.
  • The production build now emits and inspects both variants. It fails if OSS output contains Bestie, Chief of Staff, or builtin:bestie, or if the internal output lacks the protected manifest.
  • The selected variant is rebuilt last and remains in dist, preserving the normal Tauri packaging contract.
  • The protected release companion is open at squareup/buzz-releases#91. It sets VITE_BUZZ_BESTIE=1, rejects anything other than exact 1, forwards it into the signed macOS build, and adds a release-contract assertion.

Local validation on the exact Buzz head passed the full just ci gate: 5,673 Desktop tests, 2,773 Tauri tests, 1,860 mobile tests, all Rust/Tauri/web/mobile checks and builds, plus both production artifact variants. GitHub checks are running on that same head.

@wesbillman / Carl: please take a fresh look when the exact-head checks settle.

@mahanti

mahanti commented Aug 27, 2026

Copy link
Copy Markdown
Author

🤖 @buzz-security-review 523cf49

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:bot: Jude’s code review agent

REQUEST CHANGES at exact head 523cf49ced03cba9be43836a54d6aa5d6923cc82 against base eb8e97cfe74afbe560d92f3a8f470967a992767f.

The replacement architecture now enforces the intended artifact boundary: the OSS module graph excludes Bestie metadata, the internal graph includes it, and forged persisted state cannot resurrect a feature absent from the compiled manifest. However, the new build wrapper breaks the Windows production/package path.

Blocking finding

[P1] The production wrapper cannot launch Vite on Windowsdesktop/scripts/build-protected-feature-artifacts.mjs:23-41.

On Windows the wrapper selects vite.cmd and passes it directly to spawnSync() without shell or cmd.exe. Node documents that .bat/.cmd files are not executable on their own and must be invoked via a shell, exec(), or cmd.exe. This affects the shipped path: desktop/package.json:8 routes pnpm build through this wrapper, Tauri calls it via desktop/src-tauri/tauri.conf.json:13, and Windows canary/release packaging calls pnpm tauri build. Current PR CI only runs Windows Rust; the production Desktop build gate runs on macOS, so green CI does not exercise this failure.

Author action: launch Vite portably—prefer resolving its local JavaScript entrypoint and invoking it with process.execPath, or explicitly use cmd.exe /d /s /c on Windows with safe argument handling. Add a Windows production-build regression/gate that executes the real wrapper far enough to prove both variants launch and the selected dist is retained.

Verification owner: author for the portable launcher and regression; reviewer/CI for replacement-head integration and Windows freshness; release owner for companion squareup/buzz-releases#91 and signed protected-artifact verification.

Integrated exact-head evidence

  • Full Desktop unit suite: 5,673/5,673 passed.
  • Desktop typecheck passed; Desktop check passed with five pre-existing diagnostics outside PR files.
  • Both production artifact variants passed on macOS. OSS dist had no case-insensitive Bestie, Chief of Staff, or builtin:bestie match; internal dist contained the Bestie manifest marker.
  • Causal artifact mutations correctly failed the wrapper when protected content entered OSS output or the internal manifest was absent; sources were restored and the tree was clean.
  • Compiled Settings validation passed: forged cross-build {bestie:true} did not expose Bestie in OSS; internal Bestie rendered as an accessible switch, toggled by keyboard, persisted, and survived reload.
  • Exact-head product CI is green, including Desktop Core, macOS build, all smoke and integration shards, relay E2E, Release Candidate, Unit, and Security. Mark Previous Review Stale is an administrative failure, not this PR’s product defect.

Residual risk: no signed internal release artifact or native WebView was inspected. Companion release Buildkite had not reported a result when checked. Those are confidence gaps separate from the author-actionable Windows defect. Any new head invalidates this review.

Fizz and others added 2 commits August 28, 2026 10:43
On-behalf-of: mahanti <arjun.mahanti@gmail.com>
Signed-off-by: Fizz <fizz@buzz.local>
Co-authored-by: Codex <noreply@openai.com>
On-behalf-of: mahanti <arjun.mahanti@gmail.com>

Signed-off-by: Fizz <fizz@buzz.local>
Co-authored-by: Codex <noreply@openai.com>

@wesbillman wesbillman 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.

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

Verdict: REQUEST CHANGES

Reviewed exact head 523cf49ced03cba9be43836a54d6aa5d6923cc82 against base eb8e97cfe74afbe560d92f3a8f470967a992767f for the protected Chief/Bestie artifact boundary.

Blocking finding

[P1] The new production build wrapper cannot launch Vite on Windowsdesktop/scripts/build-protected-feature-artifacts.mjs:23-41.

The wrapper selects vite.cmd on Windows and passes that batch shim directly to spawnSync() without shell: true or cmd.exe. Node cannot execute .cmd files directly on Windows. This is on the shipped path: desktop/package.json:8 routes every pnpm build through the wrapper, Tauri invokes it through desktop/src-tauri/tauri.conf.json:13, and both .github/workflows/release.yml:720 and .github/workflows/windows-canary.yml:149 run pnpm tauri build. Current PR CI’s Windows lane compiles Rust only, so the green checks do not exercise this frontend/package failure.

Author action: invoke Vite portably, preferably by resolving its JavaScript entrypoint and launching it with process.execPath, or invoke the Windows shim through cmd.exe with safe argument handling. Add a Windows production-build check that executes the real wrapper and verifies that the selected dist variant is retained.

Protected-build boundary

The replacement architecture fixes my prior artifact-exclusion blocker. Vite now aliases the sole @protected-features import to an empty public module unless the value is exactly 1; the production wrapper builds and inspects both variants, leaves the selected variant in dist, and rejects Bestie/Chief markers in OSS output. The companion squareup/buzz-releases#91 strictly requires and forwards VITE_BUZZ_BESTIE=1; its Buildkite release validation is green, though that companion remains open and review-required. This is artifact selection, not an entitlement boundary: a downstream source builder can still choose the public flag.

The unrelated ACP-command persistence findings reported elsewhere are not in PR #6902’s file range and are excluded from this review.

No CI-equivalent suite was rerun locally. Exact-head CI is green for the tested product lanes; the missing Windows frontend build coverage is the material gap above.

On-behalf-of: mahanti <arjun.mahanti@gmail.com>

Signed-off-by: Fizz <fizz@buzz.local>
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is a3730784fc851bb1125b40cca9b0a30788a293c1...c55c52c2524f125bc82be0eb864d349d32644787.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review c55c52c2524f125bc82be0eb864d349d32644787 to authorize a new review.
Any previous review applies only to its recorded range.

Fizz and others added 3 commits August 28, 2026 11:10
On-behalf-of: mahanti <arjun.mahanti@gmail.com>
Signed-off-by: Fizz <fizz@buzz.local>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti

mahanti commented Aug 28, 2026

Copy link
Copy Markdown
Author

🤖 Codex here — I’m Arjun’s AI coding agent. I’m writing this review follow-up myself, not speaking as Arjun.

@jedwards27, your feedback is addressed at exact head 85b753b177f6d522d81772649fceb3a220471156:

  • The protected-build boundary no longer depends on callers remembering an optional runtime capability argument. Vite selects either the empty public protected-feature module or the internal module at build time, and the OSS graph has no Bestie definition to enable through persisted state.
  • The production wrapper now resolves Vite’s JavaScript entrypoint and launches it with Node, so it works on Windows without trying to execute vite.cmd directly.
  • CI now runs the real production wrapper for both protected-feature selections on Windows and makes that job part of the Desktop aggregate gate.
  • I also corrected a later regression that had made Bestie default-on. The internal definition is default-off again, and the checked-in test now verifies that behavior through the real feature resolver.

@wesbillman, your feedback is addressed at the same exact head:

  • OSS and internal builds now use separate module graphs. The public module is empty; Bestie metadata exists only in the internal module selected by exact VITE_BUZZ_BESTIE=1.
  • The production build emits and inspects both variants. It fails if the OSS artifact contains Bestie/Chief markers or if the internal artifact lacks the Bestie manifest, then leaves the requested variant in dist for Tauri packaging.
  • The portable Vite launcher and Windows two-variant production-build job close the packaging and coverage gap you identified.

Validation on the final branch:

  • Both production variants built successfully.
  • The OSS artifact scan found no protected markers.
  • The internal artifact contained the Bestie manifest.
  • Desktop tests, typechecking, static checks, file-size checks, branch freshness, and organization safety all passed in the pre-push gate.

Please take a fresh look at this exact head when ready.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:bot: Jude’s code review agent

APPROVE at exact head 85b753b177f6d522d81772649fceb3a220471156 against current base 58cc4b7e9be71223f7898ea5f930a4248830ccfb.

Both assigned lanes completed at this replacement head. The prior REQUEST CHANGES evidence at 63c886632162d08464b865be3147ec42508cbb1f is expired and was not carried forward. No unresolved author-actionable systems, product, UI, accessibility, artifact-boundary, security, relay, persistence, or lifecycle defect remains.

Former blocker resolved

Commit 73085c8ce removes defaultEnabled: true; the internal Bestie definition again has no default-on value. The new regression resolves the actual protected definition through resolveEnabled(..., {}, defaultEnabled) and asserts false (desktop/src/protectedFeatures/internal.ts:4-10; protectedFeatures.test.mjs:13-20). Explicit stored overrides still win, so an internal build requires user opt-in.

OSS remains absent-by-construction: only literal VITE_BUZZ_BESTIE === "1" selects the internal module; otherwise Vite aliases the empty public manifest. Forged persisted settings cannot create a feature missing from that manifest. The portable artifact wrapper runs Vite's JS entrypoint under process.execPath, builds/scans both variants, rejects protected markers in OSS, requires the internal marker, and retains the selected dist.

Exact-head evidence

  • Focused protected-feature regression: 2/2 pass.
  • Desktop typecheck: pass.
  • Isolated internal production build and marker scan: pass.
  • git diff --check: pass; clean exact-head trees.
  • Exact-head CI already green for DCO, Security, Rust lint, both server cross-compiles, Desktop Windows Build, Desktop Release Candidate, path/dead-token, and stale-review gates.

Unit, Desktop Core, macOS build, smoke 1–4, relay E2E, Mobile, and Windows Rust were still running at the final poll. That is a CI-owned confidence gap, not author rework unless a PR-caused failure emerges. No signed protected release artifact/native WebView journey was independently inspected.

Author action: none.
Verification owner: CI/integration for pending exact-head jobs; release owner for companion buzz-releases#91, flag injection, and signed protected-artifact audit.

Any new head invalidates this approval.

@mahanti

mahanti commented Aug 28, 2026

Copy link
Copy Markdown
Author

@buzz-security-review 85b753b

@wesbillman wesbillman 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.

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

Verdict: REQUEST CHANGES

Reviewed exact head 85b753b177f6d522d81772649fceb3a220471156 against base/merge-base 58cc4b7e9be71223f7898ea5f930a4248830ccfb.

[P2] Explicitly disable the flag in the OSS child instead of deleting it

Location: desktop/scripts/build-protected-feature-artifacts.mjs:33-38, interacting with desktop/vite.config.ts:10-12.

The OSS branch deletes VITE_BUZZ_BESTIE from the child's environment. That does not force the public graph: Vite's new loadEnv() fallback reloads VITE_BUZZ_BESTIE=1 from desktop/.env.local (likewise applicable production env files). Both children therefore build the internal graph, and the artifact assertion rejects the supposed OSS output. This breaks production/package builds for developers configuring the protected build through the env-file path that the Vite config explicitly supports, even if the parent process also explicitly sets VITE_BUZZ_BESTIE=1.

Reproduction from the repo root, with dependencies installed:

printf 'VITE_BUZZ_BESTIE=1\n' > desktop/.env.local
VITE_BUZZ_BESTIE=1 node desktop/scripts/build-protected-feature-artifacts.mjs

Use a disposable checkout; do not overwrite an existing local env file. Both Vite builds complete, then the wrapper exits 1 with:

Error: Official OSS desktop artifact contains protected Bestie/Chief content

pnpm build and Tauri packaging use this wrapper. This is a build-availability defect, not an artifact leak: the scanner correctly fails closed.

Requested fix: pin each child to an explicit variant (0 for OSS, 1 for internal), and make the wrapper's requested-output selection consistent with the env sources accepted by Vite. Add a regression with a file-based =1 value, including process overrides, that checks both artifacts and the variant retained in dist.

Validation

On the exact head with frozen-lockfile dependencies and the repo's Hermit Node toolchain:

  • Without the local env file, the real wrapper passed for both requested variants and retained the expected dist.
  • Adding the env file reproduced the failure above. Changing only the OSS child's deletion to an explicit 0 made the identical internal-build invocation pass. The mutation and env fixture were removed; the tracked worktree is clean.
  • Exact-head GitHub CI run 33189770270 is successful, including Desktop Core, macOS build, Windows production matrix, smoke and integration lanes. Those clean-checkout jobs do not cover the file-based configuration above. No broad CI-equivalent suite was rerun locally.

Earlier findings

The prior artifact-exclusion and Windows-launch blockers are resolved. The public graph is empty, the internal definition is default-off again, and the wrapper launches Vite's JavaScript entrypoint via process.execPath. No Bestie product surface is introduced by this PR.

Both delegated lanes are complete. Focused Chromium component checks covered internal default-off, accessible keyboard toggling, persistence/reload, invalid overrides, and OSS picker omission. The existing cross-build override-pruning behavior is nonblocking. Future protected implementations must remain in the protected graph; the general unknown-feature hook is not an exclusion boundary.

The companion squareup/buzz-releases#91 has coherent flag injection/validation but is still open and review-required at 6d83401f21c3cfd3af12c9187c37fdfb6bf76448. That is a release-integration dependency, not an additional defect in this PR. No signed internal artifact or native WebView journey was independently validated here.

The new finding is limited to file-configured local production builds; clean CI/release checkouts are unaffected. I am retaining P2 because this is a deterministic failure of the newly supported build configuration, not merely an error-message improvement. It is a small fix, but the current build cannot complete in that supported configuration.

mahanti and others added 2 commits August 28, 2026 16:06
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
@mahanti

mahanti commented Aug 28, 2026

Copy link
Copy Markdown
Author

🤖 Codex here — I’m Arjun’s AI coding agent, writing this follow-up myself rather than speaking as Arjun.

@wesbillman, your exact-head .env.local finding is addressed at c55c52c2524f125bc82be0eb864d349d32644787:

  • Each child build now receives an explicit VITE_BUZZ_BESTIE value: 0 for OSS and 1 for internal. The OSS child can no longer reload =1 from .env.local or a production-mode env file.
  • The wrapper now uses Vite’s loadEnv("production", desktopRoot, "") result with the same process-over-env-file precedence as vite.config.ts when deciding which requested variant must remain in dist.
  • The artifact-matrix orchestration is testable without touching a developer’s environment files.
  • The new regression creates a temporary .env.local containing VITE_BUZZ_BESTIE=1, verifies the file selects internal, verifies a process =0 override selects OSS, checks both artifact contents, checks build order, and confirms the requested variant is retained in the selected output.

I also reproduced your original failure before the change using a disposable local fixture. After the change, the real production wrapper passed with .env.local=1 for both VITE_BUZZ_BESTIE=1 and a process override of 0; the internal dist contained the manifest and the OSS dist contained no protected markers. The fixture was removed.

Final merged-head validation passed:

  • Focused protected-feature tests: 3/3
  • Full Desktop test suite
  • Desktop typecheck and static checks
  • Production artifact matrix
  • File-size, branch-freshness, and organization-safety pre-push gates

Please take a fresh look at this exact head.

@mahanti

mahanti commented Aug 28, 2026

Copy link
Copy Markdown
Author

🤖 @buzz-security-review c55c52c

@mahanti
mahanti requested a review from wesbillman August 28, 2026 20:10

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES

Reviewed: a3730784fc851bb1125b40cca9b0a30788a293c1..c55c52c2524f125bc82be0eb864d349d32644787 (exact live head verified immediately before submission)

Risk: high — this changes the production/Tauri packaging boundary and the compile-time separation between official OSS and protected artifacts.

Blocking finding

[P1] Concurrent opposite-variant builds race on the shared desktop/dist.

desktop/scripts/build-protected-feature-artifacts.mjs:101-115,123-136 gives each process a private directory only for the alternate graph; every process builds its selected graph directly into the same desktop/dist with --emptyOutDir. Tauri then consumes that shared directory after beforeBuildCommand (desktop/src-tauri/tauri.conf.json:13-14). There is no cross-process ownership covering both production build and packaging consumption.

A real-process mutation test started OSS (VITE_BUZZ_BESTIE=0) and protected (=1) wrappers together from one clean checkout five times. Every pair produced exactly one failure; the final dist belonged nondeterministically to the winner:

RUN=1 oss_rc=0 internal_rc=1 final_dist=oss
RUN=2 oss_rc=1 internal_rc=0 final_dist=internal
RUN=3 oss_rc=1 internal_rc=0 final_dist=internal
RUN=4 oss_rc=1 internal_rc=0 final_dist=internal
RUN=5 oss_rc=1 internal_rc=0 final_dist=internal

Four OSS processes completed their own Vite child and then rejected an internal graph that the competing process had written into dist; another child failed while output was concurrently mutated. Thus a simultaneous OSS/internal Tauri or release invocation can fail, and the package input is not owned by the invocation that requested it.

Author action: introduce a portable packaging-scoped ownership design. Either serialize access to desktop/dist through the selected build, contract scan, and Tauri consumption, or build per invocation and atomically hand each requested graph to its packaging consumer. A wrapper lock released before Tauri reads dist does not close the race. Add a real-process opposite-variant concurrency regression proving that both invocations complete and each packaging consumer receives its requested graph.

Verification owner: author for ownership design/regression; reviewer for opposite-variant concurrent mutation; CI/release owner for concurrent packaging validation on the replacement head.

Other contracts verified

No additional product/UI/security defect was found. Sequential variant pinning is effective: every Vite child receives explicit 0/1; hostile .env.local and process-env precedence retained the requested graph; mutating the OSS child back to an unpinned environment failed closed. The OSS graph remained marker-free under forged env/storage, unknown persisted overrides were discarded, protected Bestie remained default-off until explicit opt-in, and the existing labeled Radix Settings switch retained its keyboard/accessibility semantics. Sequential selected-build failure/recovery cleaned the private alternate output and a later build replaced stale dist with the requested graph. Portable Vite invocation uses process.execPath; exact-head Windows and macOS Desktop build checks passed.

Validation at clean exact head

  • Full Desktop frontend suite: 5,775/5,775 pass
  • Protected artifact focused tests: 3/3 pass
  • Desktop typecheck: pass
  • Desktop check: pass (four warnings/five infos outside PR-specific files)
  • OSS and protected production builds: pass sequentially
  • Hostile env/storage probes and pin-removal mutation: pass/fail closed as intended
  • Opposite-variant concurrent real-process matrix: 5/5 pairs reproduced the race
  • git diff --check: pass
  • GitHub exact-head Desktop Windows Build and Desktop Build (macOS): success; broader exact-head CI was still running at final review poll with no observed completed failure

Confidence gaps (not additional defects)

No signed protected release artifact or native WebView/accessibility snapshot was inspected. Companion squareup/buzz-releases#91 remained open with its observed Buildkite/security checks passing and owner review pending. Release owner owns the signed-artifact/companion contract; CI owns remaining exact-head jobs; native/release QA owns any packaged Settings accessibility observation.

— :bot: Jude’s code review agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants