feat(server): offer the Onboarding Lab Preview on every staging deployment - #202
Conversation
…yment Scenario Preview is fixed client-side fixtures that read nothing and write nothing, and #191 opened it to every authenticated Account. It nevertheless stayed invisible on a staging deployment that had not configured `OPENTAG_STAGING_ONBOARDING_ACCOUNT_ID`, because that one setting decided whether the Lab existed at all: unset meant no route, so the read-only half waited on an operations change it never needed. The setting now decides only what it names, the Account that owns the reset. `ServerConfig.stagingOnboardingLab` is present on every staging deployment and carries `accountId` only once one is configured; `OnboardingResetService` takes `labAccountId` as optional and `allows` returns false for every Account until it is set, which `resetOnboarding` re-checks before any mutation. So an unconfigured staging deployment offers Preview and owns no reset: GET answers `{ reset: false }`, POST is refused exactly like a page that does not exist. Outside staging nothing changes. `enabled` still re-confirms the environment on the read path, the config still refuses the setting unless `OPENTAG_ENV` is `staging`, and production registers no route at all. Verified by booting the built Server against a real PostgreSQL. With `OPENTAG_ENV=staging` and no Lab setting the interface answers 401 to an unauthenticated request, so the route exists where it previously did not; with `OPENTAG_ENV=prod` both methods answer 404 and the route is absent. The config change is proven by reverting it, which fails the new assertion with `expected undefined to deeply equal {}`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186L5rTjLYUFmaQ8bHkGSaB
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The implementation keeps reset fail-closed, but the changed route-availability invariant was not propagated to the shared, server, and web contract comments that describe it.
Risk level: B-high
- Path baseline:
packages/server/**, includingservices/onboarding-lab/onboarding-reset-service.ts. - Semantic lift: changes Onboarding Lab availability from “configured Account required” to “Preview on every staging deployment; reset owner optional.”
PR summary
- Problem: authenticated users cannot reach the read-only Scenario Preview on staging deployments that have no reset Account configured.
- Approach: register the Lab on every staging deployment, represent reset ownership as optional, and keep both route assembly and mutation authorization closed outside the intended environment/account.
- Impacted modules: Server config and onboarding service, internal Lab route assembly, route/config tests, and operator documentation.
Review findings
❌ 1. Update every contract comment and test description that still defines “no configured Lab Account means no Lab route.” The new startup path now registers Preview on every staging deployment, but the shared schema comment, Server app/route comments, Web API/router comments, and the omitted-option route test still explicitly document the old invariant. These are the exact readers and boundaries of the state being changed, so leaving them contradictory will misdirect future callers and refactors. Please synchronize them in this PR. [R5 / packages/shared/src/onboarding-lab.ts:3, packages/server/src/api/internal-onboarding-lab.ts:13, packages/server/src/app.ts:65, apps/web/src/api.ts:253, apps/web/src/router.tsx:671, packages/server/src/__tests__/onboarding-lab-routes.test.ts:56]
✅ The mutation path remains fail-closed: absent ownership makes allows false, and resetOnboarding re-checks it before any database work; normal startup still omits the route outside staging.
Action taken
- Submitted request changes.
…riant Review found the invariant changed in code but still described in the comments and test names that define it. "No configured Lab Account means no Lab route" was true before this branch; it now misdirects every reader of the boundary. Six places now say what the code does: the shared access schema, the internal route module, the `createApp` option, the Web client method, the Web route, and the two test descriptions that named configuration where they now mean staging. The distinction they carry is that 404 marks a deployment outside staging, not one that has configured no Account, and that `reset` is false for everyone until an Account is configured to own it. Comment-only apart from renaming a test option from `configured` to `offered`, which is the property it actually controls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186L5rTjLYUFmaQ8bHkGSaB
|
@baixiaohang Fair — I changed the invariant and left its documentation behind, at exactly the boundaries a future caller reads. Fixed in
The distinction they now carry is the one this PR actually changes: 404 marks a deployment outside staging, not one that has configured no Account. Comment-only apart from that test-option rename. Generated by Claude Code |
…ew-without-account-config
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: A staging deployment is not an internal-only authorization boundary, yet this head exposes the internal Scenario Preview to every self-created Google Account with no independent viewer gate.
Risk level: B-high
- Path baseline:
packages/server/**, includingservices/onboarding-lab/onboarding-reset-service.ts. - Semantic lift: changes Onboarding Lab availability and disclosure from an operator-configured surface to an unconditional staging capability.
PR summary
- Problem: authenticated reviewers cannot reach the read-only Scenario Preview when a staging deployment has no reset Account configured.
- Approach: enable the Lab on every staging deployment, make reset ownership optional, and retain exact-owner authorization for the destructive POST.
- Impacted modules: Server config and Lab service/route assembly, shared access contract, Web route semantics, tests, and operator documentation.
Review findings
❌ 1. Do not use OPENTAG_ENV=staging itself as authorization to disclose this internal review surface. Staging may be used by external users, and the current browser-auth path accepts any verified Google identity and creates an Account when none exists. This PR then unconditionally constructs stagingOnboardingLab for that environment, while GET checks only authentication plus reset.enabled; consequently every external staging Account can render the internal Lab, its unreleased onboarding states, and its product/workflow copy. Fixed client-side fixtures make the data-integrity risk low, but they do not remove the product-information disclosure boundary, and an unlinked URL is not access control. Add a separate, explicit server-side Preview gate/viewer allowlist (default closed), or enforce an equivalent documented staging-wide identity boundary; keep it separate from the destructive reset owner. Cover disabled Preview, allowed viewer, ordinary external Account, and unchanged reset-owner behavior. [R3/R5 / packages/server/src/config.ts:345, packages/server/src/api/internal-onboarding-lab.ts:33, packages/server/src/services/auth/google-browser-auth.ts:92, packages/server/src/services/auth/identity-service.ts:133, docs/staging-onboarding-lab.md:13]
✅ The destructive path remains fail-closed: absent ownership makes allows false, and resetOnboarding re-checks it before database work.
✅ The previous contract-comment finding is resolved across shared, Server, Web, and tests on this head.
Action taken
- Submitted request changes on exact head
4c3447f97563107cb33b0c0af9b721d5132c1160.
|
@baixiaohang I am not disputing the finding. I verified it rather than argued with it, and it holds:
So on this head, "every signed-in staging Account" does mean "anyone with a Google account", and the internal onboarding states and copy would be visible to them. The author has weighed that disclosure and accepts it for this surface. The reasoning, so it is on the record rather than implied:
Point 3 is the one I would ask you to weigh: the gap you found is real and larger than this PR. Closing it at the Lab would leave the actual surface open while reading as if it were closed. The boundary belongs at staging sign-in — a domain allowlist on Account creation, which is also the "documented staging-wide identity boundary" your finding names as an acceptable alternative. That is worth its own change, and we are happy to track it separately. Given that, would you reconsider on this head? If you would still rather have the gate at the Lab first, say so and I will implement it as a default-closed viewer allowlist keyed on email domain, separate from the reset owner, with the four cases you listed covered. Generated by Claude Code |
|
Thanks for verifying the boundary directly. I agree that the larger gap belongs at staging sign-in, but I do not think that makes this PR's incremental disclosure non-blocking. Full product access and access to a curated internal review surface are not equivalent: the Lab makes unreleased states, copy, and workflow decisions directly inspectable, and this PR changes that surface from deployment opt-in to unconditional availability. The reset variable was the wrong long-term viewer gate, but removing its accidental protection still changes the current exposure. A future staging-auth change is not a control present on this head, so merging in this order would deliberately open the new surface first and close the larger boundary later. Either resolution is acceptable:
The four cases from the review still describe the required contract. I am therefore keeping the request-changes conclusion on |
|
@baixiaohang To close the question I left open: the author has confirmed this as a product decision and is not asking for the Lab-side viewer gate. So no further change is coming on this head — the disclosure is accepted deliberately, on the reasoning in my previous comment, and not by oversight. Your finding stands on the record either way, and I would rather it did: it is accurate, and the part of it that outlives this PR is that staging sign-in creates an Account for any verified Google identity and grants it a default Workspace. That is the boundary worth fixing, and it is unaffected by whether this PR merges. Over to you — approve, or hold it and say what you need. Either is fine; I am not going to re-argue it. Generated by Claude Code |
|
Decision confirmed: I am holding the request-changes conclusion on the current head. Please either land the staging-wide identity boundary first (and make this PR explicitly rely on that shipped control), or add the independent, default-closed Preview viewer gate here. I will re-review the next head; no further discussion on the current head is needed. |
3 participants
Summary
Scenario Preview is fixed client-side fixtures that read nothing and write nothing, and #191 opened it to every authenticated Account. It nevertheless stays invisible on a staging deployment that has not configured
OPENTAG_STAGING_ONBOARDING_ACCOUNT_ID, because that one setting decides whether the Lab exists at all: unset means no route is registered, so the read-only half waits on an operations change it never needed. That is the state staging is in today — the page is deployed and unreachable.The setting now decides only what its name says, the Account that owns the reset.
ServerConfig.stagingOnboardingLabis present on every staging deployment, and carriesaccountIdonly once one is configured.OnboardingResetServicetakeslabAccountIdas optional.allowsreturns false for every Account until it is set, andresetOnboardingre-checks it before any mutation, so an unconfigured deployment can never reset even if a request reached the handler.GETanswers{ reset: false },POSTis refused exactly like a page that does not exist.Outside staging nothing changes.
enabledstill re-confirms the environment on the read path rather than trusting route registration, the config still refuses the setting unlessOPENTAG_ENVisstaging, and production registers no route at all.No Web change: the route already renders Preview for
{ reset: false }and hides the reset half, which is exactly the unconfigured shape.Testing
pnpm check,pnpm build,pnpm typecheck— pass.pnpm test— Shared 70, Server 268, Web 325, CLI 124 pass.@opentag/clientfails two tests in this sandbox:codex-app-server.test.tsD-11 closes the watched provider process group including descendantsandcodex-agent-runtime-exhaustive.test.tsaborts a hanging real App Server readiness probe and closes its process. Both reproduce identically on a clean tree here (process-group teardown in the sandbox); no file in@opentag/clientis touched.GET200{ reset: false },POST404, reset never invoked), and config assertions thatstagingOnboardingLabis{}on staging with the setting empty or absent, andundefinedon dev and prod.OPENTAG_ENV=staging, no Lab setting → both methods answer401to an unauthenticated request, so the route exists where it previously did not.OPENTAG_ENV=prod→ both answer404; the route is absent.expected undefined to deeply equal {}.Breaking changes
None. No request path changes for a deployment that already configures the setting, and none outside staging. A staging deployment that configures nothing gains a read-only page it could not previously reach; the same disclosure tradeoff #191 recorded applies, now also to staging deployments with no Lab Account.
Checklist
pnpm check,pnpm build,pnpm typecheck, andpnpm testpass. (pnpm test: see the two pre-existing sandbox failures noted above.)🤖 Generated with Claude Code
https://claude.ai/code/session_0186L5rTjLYUFmaQ8bHkGSaB
Generated by Claude Code