Skip to content

docs: say a Cloud environment is protected before asking for a request - #3616

Merged
kojiwakayama merged 5 commits into
mainfrom
fix/dx-20260811-r2-30
Aug 12, 2026
Merged

docs: say a Cloud environment is protected before asking for a request#3616
kojiwakayama merged 5 commits into
mainfrom
fix/dx-20260811-r2-30

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Dogfood finding (round 2, item 30) against published 0.1.1229.

The symptom

The Deploy project page tells the reader to check the preview before deploying,
then to verify the deployment with curl -sSf <environment-url>. On a default
Veryfront Cloud project neither step does what the page implies.

Veryfront Cloud creates preview, staging, and production as protected.
A protected environment serves only a browser signed in to Veryfront as a
project member. Every other request gets a 302 to the sign-in page, on every
path including API routes:

$ curl -s -o /dev/null -w '%{http_code} %{redirect_url}\n' https://support-agent.preview.veryfront.com/
302 https://veryfront.com/sign-in?from=%2F

$ curl -s -o /dev/null -w '%{http_code} %{redirect_url}\n' https://support-agent.production.veryfront.com/api/health
302 https://veryfront.com/sign-in?from=https%3A%2F%2Fsupport-agent.production.veryfront.com%2Fapi%2Fhealth

VERYFRONT_API_TOKEN does not change that — it authenticates the CLI against
the Cloud API, not deployment traffic.

The page never said any of this, which made its own verification step worse
than useless. curl does not treat a 302 as a failure, so the page's
curl -sSf <environment-url> exits 0 with an empty body whether or not the
deployment works
:

$ curl -sSf https://support-agent.production.veryfront.com; echo "exit=$?"
exit=0        # zero bytes of output

A reader following the page sees a green exit code and no output, and concludes
the deploy is fine. Same for the /api/ag-ui POST directly below it.

The change

  • New Environment access section, placed between Push and Deploy — the point
    at which the page first asks the reader to check the preview. It names the
    default, the redirect, why the API token is irrelevant to it, and the Studio
    switch (Environments → Public Environment → Make Public) that opens an
    environment up.
  • Verify it worked now prints the status line instead of swallowing it, so a
    sign-in redirect is visible rather than passing silently, and says outright
    why the bare curl -sSf form must not be used.

Docs and one test only. No source change.

Relationship to #3576

The finding's other half — "veryfront open does not verify the deployment" —
was already fixed on main by #3576, and open's behaviour is confirmed
unchanged on the published CLI:

$ VERYFRONT_PROJECT_SLUG=support-agent veryfront open --json   # v0.1.1229
{ "command": "open", "data": { "url": "https://veryfront.com/projects/support-agent" } }

That fix has not reached the live site, so it ships to readers together with
this one.

Verification

The regression test failed first on main's copy of the page (Expected actual: "…" to contain: "protected by default"), then passed after the edit.

Proof against the original symptom, not just the test: the new command was
copied verbatim out of the built page and run against a real protected
environment, and it printed exactly what the page now says it will —

page says: curl -s -o /dev/null -w '%{http_code} %{redirect_url}\n' <environment-url>
302 https://veryfront.com/sign-in?from=https%3A%2F%2Fsupport-agent.production.veryfront.com%2F

Also green locally: deno test --no-check -A tests/docs/ (only unrelated
network-sandbox failure in guide-examples.test.ts), deno fmt --check (4992
files), deno lint, scripts/docs/validate-guides.ts,
scripts/lint/check-doc-links.ts (1232 links OK).

Live URL to check after merge

This page is synced into veryfront-docs as docs/code/getting-started/deploy-project.md
and published at:

https://veryfront.com/docs/code/getting-started/deploy-project

That page must show an Environment access section and a Verify it worked
step whose command contains %{http_code} %{redirect_url}. Note that the live
page is currently ahead of veryfront-docs@main and behind veryfront-code@main
for this file, so the sync is worth confirming rather than assuming.

Summary by CodeRabbit

  • Documentation

    • Added guidance on protected environments, authentication redirects, membership errors, API token limitations, and making environments public.
    • Improved deployment verification instructions to test an actual project route, display redirect status codes, and support projects without a root page.
    • Documented sign-in host differences for .com and .org environments.
  • Tests

    • Added documentation checks covering environment protection, redirects, sign-in hosts, and route-based deployment verification.

The Deploy project page asks the reader to check the preview before
deploying, then to verify the deployment with `curl -sSf <environment-url>`.
Neither step works as written on a default Veryfront Cloud project.

Cloud creates `preview`, `staging`, and `production` as protected. A
protected environment serves only a browser signed in to Veryfront as a
project member; every other request gets a 302 to the sign-in page, on
every path including API routes, and `VERYFRONT_API_TOKEN` does not change
that because it authenticates the CLI against the Cloud API, not
deployment traffic. Against published 0.1.1229:

  curl -s -o /dev/null -w '%{http_code} %{redirect_url}\n' \
    https://support-agent.preview.veryfront.com/
  302 https://veryfront.com/sign-in?from=%2F

The page never said so, which made the verification step worse than
useless: `curl` does not treat a 302 as a failure, so the page's own
`curl -sSf <environment-url>` exits 0 with an empty body whether or not
the deployment works. A reader following the page sees a green exit code
and no output, and concludes the deploy is fine.

Add an Environment access section between Push and Deploy — where the
reader is first told to check the preview — naming the default, the
redirect, the token's irrelevance to it, and the Studio switch that makes
an environment public. Rewrite the verification step to print the status
line, so a sign-in redirect is visible instead of passing silently, and
say outright why the bare `curl -sSf` form must not be used.

The page's other verification claim was already corrected in #3576: this
page and the sibling guide state that `veryfront open` opens the Cloud
dashboard, not the deployed site, which `veryfront open --json` on 0.1.1229
confirms (`https://veryfront.com/projects/<slug>`). That correction has not
reached the live site yet, so this change ships alongside it.

The regression test pins the three facts a reader needs — protected by
default, the sign-in redirect, and the Studio switch — plus the status-line
form of the check, and rejects a bare `curl -sSf <environment-url>` fence.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@kojiwakayama, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c14a7d61-0339-411f-a947-2d0da7722d91

📥 Commits

Reviewing files that changed from the base of the PR and between 6b23b36 and 2982e17.

📒 Files selected for processing (1)
  • tests/docs/guide-content.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b3db6abd-0919-45bf-aed0-a6a04529ab69

📥 Commits

Reviewing files that changed from the base of the PR and between 3ffb27b and 6b23b36.

📒 Files selected for processing (2)
  • docs/getting-started/deploy-project.md
  • tests/docs/guide-content.test.ts

📝 Walkthrough

Walkthrough

The deployment guide now documents protected environment access and route-specific verification. Contract tests require authentication behavior, host-dependent redirects, explicit route probing, status reporting, and API-only deployment handling.

Changes

Deploy verification documentation

Layer / File(s) Summary
Environment access behavior
docs/getting-started/deploy-project.md, tests/docs/guide-content.test.ts
The guide and tests cover protected environments, sign-in redirects, non-member responses, host-specific sign-in domains, API token limits, and public environments.
Route verification guidance
docs/getting-started/deploy-project.md, tests/docs/guide-content.test.ts
Verification now targets an explicit project route and reports status or redirects. The tests also cover projects without root pages and API-only deployments.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: kwakayama

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main documentation change: explain Cloud environment protection before requesting access.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dx-20260811-r2-30

Comment @coderabbitai help to get the list of available commands.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Sync path confirmed, since "merged doc fixes that never reached the live site" is the recurring failure mode this round.

.github/workflows/sync-docs.yml fires on push to main with docs/getting-started/** among its paths, and dispatches into veryfront-docs. This PR touches docs/getting-started/deploy-project.md, so the merge does trigger the sync rather than needing a manual copy.

Worth checking after merge, in order:

  1. veryfront-docs receives the dispatched update to docs/code/getting-started/deploy-project.md.
  2. https://veryfront.com/docs/code/getting-started/deploy-project shows an Environment access section, and a Verify it worked command containing %{http_code} %{redirect_url}.

One caveat found while reproducing: for this file the live page is currently ahead of veryfront-docs@main (live already has the Push-based flow; veryfront-docs@main still has the older short version) and behind veryfront-code@main (live still carries the pre-#3576 veryfront open verification wording). The three copies disagree in both directions, so step 2 is worth actually loading rather than inferring from step 1.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e68f671bbb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tests/docs/guide-content.test.ts Outdated
Comment thread docs/getting-started/deploy-project.md Outdated
Comment thread docs/getting-started/deploy-project.md
`lint:cwd-relative-test-reads` failed on the new case: it raised
`tests/docs/guide-content.test.ts` from 29 to 30 cwd-relative reads inside
test callbacks, and that ratchet may only shrink.

The surrounding cases in this file are the grandfathered 29. Rather than
join them, resolve the new read from `import.meta.url` — the fix the audit
header prescribes, and the one #3592 applied. Test files are separate
isolates sharing one process under `--parallel` and `src/testing/cwd.ts`
chdirs that process, so a cwd-relative read is correct only until an
unrelated file lands beside it in the same shard.

Baseline is untouched: 0 at module scope, 95 in callbacks across 21 files,
exactly as before.
Three review findings, all confirmed against source before applying.

The claim that "every other request gets a 302" was wrong for one case.
`checkProtectedProxyAccess` in `src/proxy/proxy-access-control.ts` returns
302 only while the caller is unauthenticated or its token yields no user id;
a caller signed in as a user who is not a project member gets 403 instead.
That is the more confusing failure of the two, because the browser is signed
in and the reader would suspect the URL rather than the account, so the page
now names it and says what it means.

The expected `302 …` line sat inside the `bash` fence with the command.
Pasting the rendered block ran `302` as a third command. It moves to its own
`text` fence, per the repo rule that code examples must be safe to paste.

The regression test's evidence comment recorded a real deployment hostname.
Internal hostnames are on the AGENTS.md secret-safety list, and the host was
never the point of the evidence, so it is a `<project>` placeholder now. The
comment gains the source pointer for the 403 path, and the test pins the 403
sentence so the correction cannot silently regress.
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

All three applied in ec5d984. Each verified against source before changing anything.

403 for signed-in non-members (P2) — correct, and it was the worst of the two failures to leave undocumented. checkProtectedProxyAccess returns 302 only while the caller is unauthenticated or its token yields no user id; isProjectMember failing returns { status: 403, message: "Access denied" } (src/proxy/proxy-access-control.ts). A signed-in non-member is exactly the reader who would blame the URL rather than the account, so the page now says a 403 means the account is wrong, not the URL, in both the Environment access section and the verification step. The test pins the sentence.

Output inside the bash fence (P2) — correct. The 302 … line would have run as a third command on paste. It is a separate text fence now.

Hostname (P1) — correct. Internal hostnames are on the AGENTS.md secret-safety list and the host was never the load-bearing part of the evidence; the 302 shape was. It is <project>.production.veryfront.com now, and the comment gained the proxy-access-control.ts pointer for the 403 path.

Re-ran the finding's own reproduction against the amended page afterwards, not just the test suite: the verification command extracted verbatim from the page still prints 302 https://veryfront.com/sign-in?from=… against a live protected environment, which is what the page now tells the reader to expect.

@kwakayama kwakayama added needs-human-input Maintainer action required and removed needs-human-input Maintainer action required labels Aug 12, 2026

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

Findings

  1. [medium] Handle veryfront.org sign-in redirects. docs/getting-started/deploy-project.md:68-70 states that every protected environment redirects to https://veryfront.com/sign-in. That is false for preview hosts on *.preview.veryfront.org: buildProxyAuthRedirectUrl selects https://veryfront.org/sign-in for those hosts (src/proxy/proxy-access-control.ts:187-218, covered at src/proxy/proxy-access-control.test.ts:102-120). State that the sign-in apex depends on the deployment host, and add a documentation assertion covering the .org case.

  2. [medium] Do not require a 200 from the environment root for every successful public deployment. docs/getting-started/deploy-project.md:135 presents 200 as the universal public-environment result for <environment-url>. Deployments without page routes are supported: deployment selects no readiness route when none exists (cli/shared/deployment/deploy-project.ts:1293-1304), and the focused test explicitly treats such projects as having no browser URL (cli/shared/deployment/deploy-project.test.ts:635-650). A successful API-only deployment can therefore have no successful root-page response. Tell readers to probe a route their project serves and validate that route's expected status.

Category Score
Correctness 30/40
Tests 16/20
Reliability/security 14/15
Maintainability 14/15
Scope/docs 8/10
Total 82/100

Review-Gate:
Reviewer: Codex
Reviewed-SHA: ec5d984
Score: 82/100
Actionable-Findings: 2
Verdict: REQUEST_CHANGES

@kwakayama kwakayama added the needs-human-input Maintainer action required label Aug 12, 2026
@kojiwakayama
kojiwakayama added this pull request to the merge queue Aug 12, 2026
@kwakayama
kwakayama removed this pull request from the merge queue due to a manual request Aug 12, 2026
# Conflicts:
#	tests/docs/guide-content.test.ts
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Both findings verified against the source before acting. Both citations are accurate; nothing to push back on.

Pushed as 6b23b36d (which also merges origin/main — the branch had gone CONFLICTING on tests/docs/guide-content.test.ts, where #3605's open --site case and this PR's protected-environment case were appended at the same spot. Both cases kept).


Finding 1 — sign-in apex depends on the deployment host

Citation confirmed. resolveSignInApex (src/proxy/proxy-access-control.ts:187) picks the apex from the request host, and buildProxyAuthRedirectUrl (:202) uses it. *.preview.veryfront.org resolves to veryfront.org, asserted at src/proxy/proxy-access-control.test.ts:102-120. The page's flat claim was wrong.

One nuance I kept the prose honest about rather than inverting the rule: isHostedProductionDeployment short-circuits before the apex loop, so *.production.veryfront.org still signs in at veryfront.com (deliberate, per the #1827 comment at :186-190). So the page does not say "the apex follows the environment's own TLD" — that would be a new false statement. It says the apex depends on the serving host, gives the two concrete cases, and tells the reader to read the printed redirect_url rather than memorise one URL. It also now explains why it matters (cookie is scoped to the issuing domain, so signing in on the wrong apex loops).

Finding 2 — no universal 200 from the environment root

Citation confirmed, and it goes one step further than the finding states. readinessRoute is null when no non-dynamic page route exists (cli/shared/deployment/deploy-project.ts:1293), and buildEnvironmentReadinessProbes returns [] for a null route (:875) — so such a deployment is verified with zero HTTP probes, which is exactly what deploy-project.test.ts:635-650 asserts (assertEquals(requests, 0)). Note the null case is broader than "API-only": a project whose only page route is dynamic (/blog/[slug]) also filters out, since the find excludes any route containing [.

"Verify it worked" now tells the reader to probe a route their project actually serves and validate that route's expected status; the sample command takes <environment-url>/<route> instead of the bare root.


The assertions, and proof they fail first

Two new cases in tests/docs/guide-content.test.ts. Taking the note about the earlier structurally-weak assertion seriously, both are pinned to structure rather than keywords:

does not promise one sign-in apex for every protected environment collects every sign-in URL the page prints and requires both apexes to be present:

const signInApexes = new Set(
  [...doc.matchAll(/https:\/\/(veryfront\.(?:com|org))\/sign-in/g)].map((m) => m[1]),
);
assertEquals(signInApexes.has("veryfront.com"), true);
assertEquals(signInApexes.has("veryfront.org"), true);

Prose naming only .com cannot satisfy this — which is the point, since the pre-existing assertStringIncludes(prose, "https://veryfront.com/sign-in") passes happily on the broken page and so could never have caught this.

does not require a 200 from the environment root to call a deploy verified pins the command shape, because a command ending at the bare environment URL is the defect:

assertStringIncludes(doc, "-w '%{http_code} %{redirect_url}\\n' <environment-url>/<route>");
assertEquals(doc.includes("-w '%{http_code} %{redirect_url}\\n' <environment-url>\n"), false);
assertEquals(prose.includes("A public environment answers `200`."), false);

Red confirmed before the fix. I restored the pre-fix page from the index and ran the file — both new cases failed, and the apex-set assertion is what fires first:

does not promise one sign-in apex for every protected environment ... FAILED (2ms)
does not require a 200 from the environment root to call a deploy verified ... FAILED (0ms)

error: AssertionError: Values are not equal.                     # signInApexes.has("veryfront.org")
error: AssertionError: Expected actual: "---\n" to contain:
  "-w '%{http_code} %{redirect_url}\n' <environment-url>/<route>"

Restoring the fixed page: ok | 1 passed (33 steps) | 0 failed.

Gates

  • deno test --no-check -A tests/docs/52 passed | 1 failed; the only failure is the sandboxed-network one in guide-examples.test.ts (General SOCKS server failure reaching 93.184.216.34), same environmental exclusion noted in the PR body. Identified by failure text, not by test name.
  • deno fmt --check — 5007 files clean; deno lint clean.
  • scripts/docs/validate-guides.ts — all guides pass (the one webhook index warning is pre-existing on main).
  • scripts/lint/check-doc-links.ts — all 1233 links OK, including the #environment-access anchor the new cross-reference uses.
  • Full pre-push hook green; committed and pushed with Deno 2.7.7 first on PATH.

Leaving the merge decision with you — not re-queueing.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

CI follow-up: green, and one more commit.

2982e1747 — the pre-existing tells the reader a Cloud environment is protected… case still had a comment asserting the universal veryfront.com apex, which now contradicted the case sitting right below it. Comment only; the assertions are unchanged.

Flake, not a regression. The first run on this SHA came back with coverage shard 4/8, coverage gate and tests (unit) red. Those are one failure, not three — tests (unit) is the Require all unit test shards aggregator (COVERAGE_SHARDS_RESULT: failure, 3s) and coverage gate depends on the shard artifact that never uploaded.

The shard log contains no FAILED test at all. The only error is a runtime-level abort:

error: Promise resolution is still pending but the event loop has already resolved

A dangling promise, no assertion. Two independent reasons it cannot be this branch:

  1. Shard 4 runs with --ignore=tests, so tests/docs/guide-content.test.ts is not in its file list. The only other file this PR touches is a Markdown page with no runtime effect — the branch contributes nothing executable to that shard.
  2. The same shard-flakes-then-cascades shape shows up on main unprompted (run 31563881301, coverage shard 7/8coverage gatetests (unit)), there with the separate cli/commands/skills/handler.test.ts failure.

Re-ran the failed jobs only, no code change: coverage shard 4/8, coverage gate and tests (unit) all pass. Identified by failure text rather than job name, since the aggregator name is the same whatever breaks underneath.

Now: 27 pass, 0 fail, 6 skipping. Branch is MERGEABLE again after the origin/main merge.

Still not re-queued — that call is yours.

@kojiwakayama
kojiwakayama added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 7c9ea56 Aug 12, 2026
57 of 60 checks passed
@kojiwakayama
kojiwakayama deleted the fix/dx-20260811-r2-30 branch August 12, 2026 06:49
kojiwakayama added a commit that referenced this pull request Aug 12, 2026
The validator this PR adds found three em dashes that reached main while no
authoring-time check existed: two from #3606 in project-structure.md and one
from #3616 in deploy-project.md. Rewritten as sentences and parentheses with
the meaning unchanged.

The rebase also took main's 'Verify it worked' section rather than this
branch's older copy, so #3616's reviewed route-probing guidance survives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human-input Maintainer action required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants