Repository navigation
Selectable optional Gatekeepers in cf-os init, takeover and import - #21
Merged
Merged
Conversation
Upstream ships more Gatekeepers than the starter deploys, and its router and
Workshop find each one by its GATEKEEPER_* binding alone
(cloudflare-os/packages/router/src/index.ts,
workshop-backend/src/auth/auth-vendors.ts). The hosted deploy offers them from
a catalog its release tooling builds; cf-os now builds the same catalog from
the pinned submodule with upstream's own code
(cloudflare-os/scripts/release/manifest-lib.ts), loaded lazily so the published
CLI path never reaches @gadgets/*.
deployment.jsonc gains:
- "gatekeepers": { "<slug>": { "name", "vars"? } }, keyed by the hosted slug.
Each selected Gatekeeper is validated against the catalog (an excluded one
fails with upstream's reason), generated from its package's wrangler.jsonc
with BASE_URL = <origin>/gatekeeper/<slug>, built with its configurator
codegen, deployed in wave 0, and bound to the router and the Workshop
(entrypoint GatekeeperVendor).
- customGatekeeper.enabled (missing means true). false deploys no custom
Gatekeeper Worker and binds none, as errorReporting.enabled does for the
Error Reporter.
The catalog leaves out what the starter cannot deploy from a name alone:
anything upstream marks not installable (Email), anything declaring KV, R2,
service or Artifacts bindings, and anything taking another Worker's name as an
input. A package upstream's release code rejects is excluded with the reason,
so drift fails closed. The docs describe the whole feature, including what
`init` prints and what `check` warns about, which the next two commits add.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An OAuth Gatekeeper deploys without its credentials, but nobody can connect it until they are set with `wrangler secret put`; deploys keep them once set. Wrangler's secrets.required would refuse the first deploy outright, so the check stays in cf-os and only warns. CloudflareClient.listWorkerSecrets reads a Worker's secret names (`cf workers secrets list`), null for a Worker the account does not have yet (API code 10007, as Wrangler reads it). gatekeeperSecretWarnings turns each missing input the catalog declares into a warning on stderr with the command that sets it and the redirect URI to register, or a "not deployed yet" note. `check` and `deploy` print them after the account checks, `deploy` again afterwards for whatever is still missing; neither ever fails on one, and a signed-out or unavailable client skips them as the account checks do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
init asks "Which Gatekeepers?" after the model providers: the example custom Gatekeeper, preselected, then every Gatekeeper the catalog offers, hinted with the credentials it needs. The custom Gatekeeper's display text is asked only when it stays selected. A selected Gatekeeper is named <router>-gk-<slug>, as the hosted flow names it, steered around taken names, and written as gatekeepers.<slug>.name only, so hand-added vars survive; a deselected one is removed. Once the file is written, init prints each OAuth Gatekeeper's setup: the provider's console, upstream's steps (deploy-inputs.json), the redirect URI and the `wrangler secret put` commands. Without a terminal the question fails naming its config equivalent, gatekeepers and customGatekeeper.enabled. A takeover classifies the hosted instance's <instance>-gk-* Workers: one the catalog offers is preselected and keeps its name and secrets; a repeat install (google2) or an unknown one is reported as not deployable by the starter. Deselecting a live Gatekeeper, the custom one included, warns that its binding goes and users' connections with it, while the Worker keeps running. `import` writes gatekeepers.<slug>.name for each deployable one and lists the rest with the reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
LGTM! |
A substring check on a URL reads to CodeQL as incomplete URL sanitization. Matching the whole printed line says the same thing more strictly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g again The warnings before the build already give each missing secret's command and redirect URI, and a deploy sets no secrets, so repeating them in full after the deploy doubled the output for nothing. The deploy now ends with one line saying they still apply and that `pnpm cf-os check` repeats them, with no second round of lookups. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| "redirect URI, which follows the public origin: if the instance moves hostname, add the new one." | ||
| : "")); | ||
| } | ||
| const needed = selected.filter(({ slug, offered }) => offered.secrets.length && !isLive(slug)); |
There was a problem hiding this comment.
[P2] Surface the MCP portal's required configuration in init
On a fresh config, selecting mcpportal writes only its Worker name. Its catalog entry has no secrets or declared var inputs, so this filter skips all setup guidance, no placeholder remains, and init immediately offers to deploy. However, the pinned upstream gatekeeper-mcp-portal/src/config.ts returns null without MCP_PORTAL_URL, and its vendor's getSupportedResources() consequently returns [], hiding the selected connector from the app. I reproduced the generated configuration passing validation with no portal URL or warnings. Collect gatekeepers.mcpportal.vars.MCP_PORTAL_URL, or leave an actionable placeholder/setup reminder, including when adding the portal during takeover.
Member
Author
There was a problem hiding this comment.
Confirmed and fixed in d0684bb. Upstream's portal reads MCP_PORTAL_URL (gatekeeper-mcp-portal/src/config.ts, readPortalConfig) but declares no deploy input for it, so the starter now carries it in its own UNDECLARED_VARS table:
initasks for the endpoint whenevermcpportalis chosen and the file has no usable value: new deployments, reruns and takeovers, including a portal the account already runs, since a deploy replaces its plain vars. It applies upstream's rule (https, no username or password); left empty, it stays unset.checkanddeploywarn in one line while it is unset or unusable, without failing.- A test fails if upstream stops reading the var or starts declaring an input for it, so the starter's entry can go.
|
Review: 1 findings. Posted 1 actionable inline comment. Validation: 16 focused tests passed. Broader tests were unavailable because the submodule and its dependencies are not installed in this checkout. |
The warnings before the build already give each missing secret's command and redirect URI, and a deploy sets no secrets, so nothing about them needs saying again once the deploy is done. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Since last review: 0 resolved, 1 still open, 0 new. The existing MCP portal setup finding remains unresolved. No additional actionable issues found. Validation: 16 focused tests passed. Full checks were unavailable because the submodule and dependencies are not installed in this checkout. |
init asks for an OAuth Gatekeeper's credentials (masked) once "Deploy now?" is answered yes, and the deploy uploads them with `wrangler secret bulk` on stdin after it has created the Worker, confirming each by listing the Worker's secrets again. A Gatekeeper the account already runs is asked about and set right away. `pnpm cf-os secrets [<slug>...]` sets them later; it never uploads to a Worker that does not exist, so Wrangler creates no placeholder. Without a terminal it fails naming the `wrangler secret put` commands. check and deploy shrink the missing-secret warning to one line naming `cf-os secrets`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Since last review: 0 resolved, 1 still open, 0 new. The existing MCP portal setup finding remains unresolved. I confirmed that selecting it still omits Validation: 18 focused tests passed. Full checks were unavailable because the submodule and dependencies are absent from this checkout. |
Upstream's MCP portal Gatekeeper hides itself without MCP_PORTAL_URL, but declares no deploy input for it, so choosing the portal wrote only its Worker name and offered to deploy a connector nobody could see. init now asks for the endpoint (https, no credentials in the URL, as upstream's readPortalConfig requires) whenever the portal is chosen and the file has no usable one, including on a takeover; left empty, it stays unset. check and deploy warn in one line while it is unset or unusable. A test pins that upstream still reads the var and still declares no input for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
LGTM! |
init asks up to twenty questions before it writes the deployment file, and one wrong answer meant Ctrl-C and starting over. Now Esc asks the previous question again, its earlier answer the default; Ctrl-C still quits. Clack cancels on Esc and Ctrl-C alike, so a prompter's backable view notes which key it was and throws Back for Esc. withBack (back.ts) runs the questions again on Back, replaying the answers before the previous question without asking and muting what they print. init's reads between them go through a per-run cache, so the way back costs no cf call, and the Access step now asks both its questions before it creates the One-time PIN provider or the application, so no Esc steps back over something created. The sign-in offer before the account question, and everything after the write (Wrangler sign-in, secrets, the deploy question), keep Esc as cancel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
LGTM! |
Going back left the abandoned question drawn as clack draws a cancelled one, a red square and its answer struck through, followed by a "Back to:" line: it read as cancelling. The backable prompter now counts the rows clack moves the cursor down while it draws (clack wraps frames to the terminal itself and moves only by newlines and CSI A and B), and on Esc erases back to where the prompt began. The previous question is asked again right below its earlier answer, so the "Back to:" line goes. At the first question there is nothing to go back to, so it is asked on the plain prompter, where Esc cancels and exits 130 as anywhere else. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
LGTM! |
The Workers are listed once a base name is typed, and zones only for a custom domain, so neither path waits on a read it does not use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`cf` runs asynchronously, so the event loop turns while it works. At a terminal, a step blocked on a read, a create or a sign-in check shows a spinner naming what it waits on; Ctrl-C there exits 130 with "Cancelled." Without a terminal nothing is drawn. init starts the reads a path is certain to need right after the answer that chooses it (the Workers, the Zero Trust organization and Access applications after "A new deployment"; the AI Gateways and identity providers once the base name is settled; the takeover inventory while the instance is picked), through one cache that Esc's replays also use. import, check, deploy and secrets run independent reads at once and report them in the same order as before, whichever finishes first. On a `cf auth login` sign-in, the first `cf` call after an idle spell runs alone, since it may spend Cloudflare's single-use refresh token. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review: 1 findings. Posted 1 actionable inline comment. Validation: 7 focused tests passed, and the reported behavior was reproduced against the ownership helper. Broader checks were unavailable because dependencies and the submodule are absent from this checkout. |
init lists the accounts beside its cf sign-in check, the Workers while it asks what is being set up, and starts `wrangler whoami` at the outset rather than after the file is written. Scaffolding checks gh's sign-in while `pnpm install` runs. `cf-os access` lists the Access applications while the organization is read, and check/deploy read the Gatekeeper secrets beside the account check, still printing them after it. A background check no longer needed is left to finish on its own rather than killed, without holding the process open: `wrangler whoami` may be refreshing a single-use OAuth token. `cf auth login` and Wrangler's sign-in and `secret bulk` run as `node <entry>` when the project has the CLI installed, through upstream's resolver loaded lazily, so init and secrets still load without the submodule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A `cf` still running when cf-os exits, cancelled or failed, may be refreshing its OAuth access token with the single-use refresh token, and one cut off mid-refresh leaves `cf` signed out. So exiting no longer kills it, and off Windows it runs in a process group of its own, out of reach of a terminal's Ctrl-C. Exiting still does not wait on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
localBin's `pnpm exec` fallback goes through cf-os's pnpmCommand, as cf-client.ts and deploy.ts do, so Windows spawns pnpm without a shell. It is loaded lazily beside upstream's resolver; without the submodule, a bare `pnpm exec` stands in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Going back a question in init no longer erases the one left. It stays drawn as clack draws a cancelled prompt, its answer struck through, but with a grey square rather than a red one, followed by a grey "Rewound." line, and the previous question is asked again below. Ctrl-C still cancels in red. The Esc listener now runs ahead of clack's, so the frame clack draws for the key is recoloured as it is written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
import and init's takeover took a deployment file as the instance's own, and bound the Gatekeeper and custom Gatekeeper Workers it names, whenever its workers.router.name was the instance. A file for a router of the same name in another account (--account, or another account picked) then bound or overwrote Workers of those names in this one. The file must also carry this account's ID; otherwise its Workers are left alone, as for another router's file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The scripts the tests run with `node -e` no longer have the temporary path spliced into their code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
LGTM! |
|
LGTM! |
ndisidore
approved these changes
Oct 20, 2026
ndisidore
left a comment
ndisidore
left a comment
Member
There was a problem hiding this comment.
Only low and nit stuff found so lgtm with some optional cleanup!
Findings, ranked
┌───┬─────────────────┬───────────────────────────────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────┬──────────────────────────────┐
│ # │ Sev │ Where │ Problem │ Fix │ Status │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 1 │ Low │ commands/import.ts:201-206, │ Running import --out with an account from --account or the picker writes accountId. The workers.router.name is already the instance, so the │ When the file wasn't the instance's, also reset │ I confirmed in the code that │
│ │ │ fileIsInstance at :47-52, used in │ file now passes fileIsInstance. But only the gatekeepers block is removed: workers.customGatekeeper.name, which discovery never writes, │ workers.customGatekeeper.name (and any other names │ only the gatekeepers are │
│ │ │ init.ts:497-513 │ keeps the other account's value. A later init takeover trusts it as ownCustom and skips the "Already in the account… is X this deployment's │ discovery didn't find again) and list them in the note. │ removed; the full scenario │
│ │ │ │ own?" confirmation, so the deploy can overwrite an unrelated Worker of that name. That is what commit f41768d was meant to prevent. │ │ wasn't run. │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 2 │ Low │ tools.ts:34-36, 88 (defaultCapture) │ The wrangler whoami started ahead of time is spawned without detached, so it stays in the terminal's foreground process group. Ctrl-C during │ Add the same detached option. │ I confirmed tools.ts has no │
│ │ │ │ cf auth login can then kill it while it's refreshing a single-use OAuth token, which signs the user out. Its own comment says it isn't │ │ detached. │
│ │ │ │ killed, and cf-client.ts:78 already spawns with detached: process.platform !== "win32" for this reason. │ │ │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 3 │ Low, not yet │ core/wizard.ts:101, init.ts:584-608, │ init shows a catalog Gatekeeper's vars in its hint but never asks for them. askUndeclaredVars only covers UNDECLARED_VARS, and │ Ask for catalog.offered[slug].vars, or write │ Reviewer only │
│ │ reachable │ core/deployment.ts:503-507 │ validateGatekeepers then throws on the missing var. No upstream package declares a var input today, so this only breaks after a future │ placeholders so the "Deploy now?" offer is blocked. │ │
│ │ │ │ upstream update. │ │ │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 4 │ Doc │ .agents/skills/cloudflare-os-operator/ref │ Says a new var input "makes check ask for" it. check never prompts; it fails. Same cause as #3. │ Reword to say check fails until the var is set. │ Reviewer only │
│ │ │ erences/updating.md:43 │ │ │ │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 5 │ Nit, doc │ README.md:50 │ Still says "seven Workers". The custom Gatekeeper is now optional, and AGENTS.md and docs/customization.md already say "up to seven". │ "up to seven Workers" │ Confirmed │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 6 │ Doc, wasn't │ README.md:60 │ Offers "the email Gatekeeper, which needs a zone". The new catalog excludes gatekeeper-email as not installable, and │ Drop or reword the email Gatekeeper mention. │ Confirmed │
│ │ changed by this │ │ docs/customization.md:82 says so. │ │ │
│ │ branch │ │ │ │ │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 7 │ Test, can flake │ main.test.ts:247-248, │ They depend on wall-clock margins: 1800 ms against a 2000 ms timer, 1400 against 1500, and a 20 ms fake delay. A loaded CI runner can break │ Use much longer child timers and drop the elapsed-time │ Reviewer only │
│ │ │ tools.test.ts:75-76, │ them. │ asserts; in the init test, hold the gateway read on a │ │
│ │ │ init.test.ts:1549-1551 │ │ promise resolved by hand. │ │
├───┼─────────────────┼───────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────┼──────────────────────────────┤
│ 8 │ Test, nit │ local-bin.test.ts │ Passes with or without commit 422f01d on Linux, so it can't catch a regression. On Windows it would fail, because it hard-codes "pnpm". │ Assert against pnpmCommand([...]) instead of a literal. │ Reviewer only │
└───┴─────────────────┴───────────────────────────────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────┴──────────────────────────────┘
Where the reviews overlap
- Account trust (#1). One reviewer found the gap; the other two independently confirmed the in-process paths are sound. init checks trust in all three places, a takeover drops entries from another deployment, and cached reads are keyed by accountId. So the hole is specifically the separate import
--out → init sequence.
- Process handling (#2). The Cloudflare-reads reviewer confirmed cf-client's detached spawn and its "leave a read to finish" behaviour. The init reviewer found that tools.ts doesn't do the same. It's one convention applied in one place and not the other.
- Docs. #3–#6 all come from behaviour changing in this branch without every line that describes it changing too, which AGENTS.md says must happen in the same commit.
Checked by at least one reviewer, no issues
- Secrets: values reach wrangler secret bulk only on stdin, each upload is confirmed by listing the secrets again, and error messages carry names only.
- Output and no-terminal runs: stdout and stderr are kept apart, and without a terminal a missing answer fails naming the flag.
- Deploy: wave order and cleanup are correct, and the redirect URIs match the Gatekeepers'.
- Esc-back: answers are trimmed correctly when going back, and Esc cancels at the first question and after the file is written. Nothing is created inside the part Esc can rewind.
- Read-ahead: speculative reads can't cause unhandled rejections, and the cache is rebuilt after sign-in.
- Code boundaries: src/core has no I/O, and main.ts doesn't reach @gadgets/*.
- Credentials: tokens never appear in logs or errors.
#1 and #2 are each a few lines to fix. #1 should get a test that runs import --out from a foreign account's file and then the init takeover.
… on import --out Discovery never writes workers.customGatekeeper.name or workers.errorReporter.name, so importing over another deployment's file kept its names, and once --out wrote the instance's router and account the file read as the instance's own: a deploy or a later takeover would deploy over that deployment's Workers. --out now sets them back to placeholders, listing them, so the deploy waits until init asks for this deployment's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… test timing A status read (`wrangler whoami`, `gh api user`) started ahead runs, off Windows, in a process group of its own, as `cf` does: a Ctrl-C while a sign-in holds the terminal no longer reaches one that may be refreshing a single-use OAuth token. The exit tests give their child processes 4 s and check that the run ended before them rather than against a wall-clock margin, and init's held reads wait 300 ms rather than 20. localBin's fallback is checked against pnpmCommand rather than a literal `pnpm`, which Windows differs from. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…new var input The custom Gatekeeper is optional, so the deployment is up to seven Workers; the email Gatekeeper is not installable, so the README no longer offers it; and a new `var` input makes `check` fail until it is set, which init does not ask for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
|
LGTM! |
…ne place per fact docs/customization.md#optional-gatekeepers is where operators read the details, and packages/cf-os/README.md's table is where the CLI's are. Everywhere else gets a short pointer: deployment.jsonc, the migration guide, the operator skill and its references. In source, each comment's rationale lives once (the single-use refresh token on firstAlone, the instance's-own-file rule on fileIsInstance), and the tests share one reversing() helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
LGTM! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bunch of improvements to
cf-osCLI, see below:You can test it out via
npx cf-os@latest init --ref gatekeeper-selectionCommits
cloudflare-os/scripts/release/manifest-lib.ts, loaded lazily so the published CLI path never reaches@gadgets/*.deployment.jsoncgains:"gatekeepers": { "<slug>": { "name", "vars"? } };customGatekeeper.enabled(missing meanstrue).wrangler.jsoncwithBASE_URL = <origin>/gatekeeper/<slug>.This means we can now easily deploy/adopt public repo's gatekeepers, and add them in our deployment.jsonc
You'll also get prompted to add secrets for gatekeepers you added, which you can choose to add right now, or not.
init.Don't have a screenshot, but trust me it's much faster!