Unblock the docs sync and backfill the mirror-only pages - #3627
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request adds public-documentation validation to CI and updates documentation for development URLs, MCP ports, deployment access, installation, project structure, generated directories, and external links. ChangesPublic documentation validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant DenoTasks
participant PublicDocsValidator
participant PublicDocs
CI->>DenoTasks: run lint:ci
DenoTasks->>PublicDocsValidator: run docs:public:check
PublicDocsValidator->>PublicDocs: validate required wording and coverage
PublicDocsValidator-->>CI: report documentation issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 820a74ea41
ℹ️ 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".
kwakayama
left a comment
There was a problem hiding this comment.
Findings
- [P2] Correct the protected-environment access contract in
docs/getting-started/deploy-project.md:91-108anddocs/guides/deploying.md:134-145. They state that only a signed-in browser can access a protected environment and that every other request gets a302sign-in redirect. The deployment client supports authenticated non-browser probes via anauthTokencookie (cli/shared/deployment/deploy-project.ts:850-905) and explicitly treats a sign-in redirect,401, and403as valid protection challenges (cli/shared/deployment/deploy-project.ts:927-994). The existing test confirms a programmatic protected-environment request authenticates with the stored session token (cli/shared/deployment/deploy-project.test.ts:653-669). Document these as possible authentication responses and distinguish an API key from a user session cookie.
Score breakdown: correctness 34/40, tests 17/20, reliability/security 13/15, maintainability 14/15, scope/docs 6/10.
Review-Gate:
Reviewer: Codex
Reviewed-SHA: 629746b
Score: 84/100
Actionable-Findings: 1
Verdict: COMMENT
The veryfront-docs "Update Code Docs" workflow has failed on every run since 2026-05-27. It fails at "Validate synced code docs quality" with 13 issues, so no documentation change made here has reached veryfront.com in ~2.5 months. That validator runs on veryfront-docs' docs/code/** *after* the sync overwrites that tree from this repo's docs/, so the failures are ours. Fix the source: - create-project, quickstart: show the URL `veryfront dev` actually prints (`✓ Ready in <duration>` then `http://veryfront.me:3000` on its own line), and drop the `Ready on http://localhost:3000` line the CLI never prints (cli/commands/dev/command.ts:329). - installation, coding-agents: `veryfront dev` only lists the MCP address under --verbose, so state the address instead: dev `--port` + 2 (default 3002), path `/mcp` (DEV_MCP_PORT_OFFSET, cli/commands/dev/command.ts:242). - deploying: replace an em dash with ASCII punctuation. - deploy-project, deploying, quickstart: state that Cloud environments are protected by default, that an unauthenticated request is redirected to sign-in, that VERYFRONT_API_TOKEN does not open a protected environment, and name the "Public Environment" Studio switch. Verified against cli/commands/deploy/command.ts:171 ("Protected · Release <version>") and src/proxy/proxy-access-control.ts:218 (the /sign-in?from=... redirect). Backfill the work that exists only in the mirror. While the sync was broken, 20 docs PRs were merged directly into veryfront-docs to get fixes onto the live site. A repaired pipeline would silently revert them on its first green run. Diffing a local replay of the sync against veryfront-docs origin/main isolates the content that exists only downstream: - installation: the blank-install wiring steps (module type, CLI scripts, base tsconfig, first page). The published package ships tsconfig.json and react/react-dom as dependencies (scripts/build/build-npm-dnt.ts:313,337). - project-structure: the `.cache/` and `dist/` generated-directory reference, including the self-ignoring `.cache/.gitignore` and VERYFRONT_CACHE_DIR (src/utils/cache-dir.ts:154). - providers: an absolute GitHub URL for the ext-llm-openai reference; the relative path escaped the docs tree and 404s once published. - deploy-project: replace the curl-based verification, which only ever proved the sign-in redirect, with a check that works against a protected environment. The rest of the drift is this repo legitimately ahead of the mirror (`open --site`, `install --target`, `npx veryfront@latest`, `--output`, tasks/schedules/webhooks/evals discovery, skills authorization), so it is left to overwrite the mirror as intended. Close the hole that let this happen. scripts/docs/validate-public-docs.ts already had the em-dash rule, but it only runs under `deno task docs:validate`, which CI never invokes -- CI's lint shard runs `lint:ci`. The accuracy contract was enforced only downstream, in another repo, on a repository_dispatch run nobody watches. Mirror the deploy-access, dev-server-URL, and MCP-endpoint coverage checks into the local validator and add it to `lint:ci` as `docs:public:check`, so a PR that breaks the sync fails here instead.
The dev server computes MCP as `started.port + 2`, where `started.port` is the port `startDevServerOnFreePort` actually bound after scanning forward (cli/commands/dev/command.ts:240-243). Documenting the offset against the requested `--port` sends a reader to the wrong endpoint whenever the requested port is taken: `--port 4000` that falls forward to 4001 serves MCP on 4003, not 4002. State the rule against the bound port on all three pages that carry it, and tell readers to take the port from the URL the dev server printed. Update the validator message to match. Also record why the wrapped MCP pattern stays byte-identical to the rule in veryfront-docs' check-code-docs-quality.mjs. The pattern is broad enough to reject an accurate sentence about `--verbose` output, but this validator exists to predict the downstream one; narrowing it here alone would let a page pass locally and still break the sync, which is the failure this check was added to prevent. Changing it means changing both repositories together.
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.
ba9db71 to
87c73af
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/docs/validate-public-docs.ts (1)
244-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for public-document validation.
The new
WRAPPED_RULESandcollectCoverageIssuespaths have no direct regression tests. Cover wrapped MCP text, missing deploy requirements, and missinghttp://veryfront.me:3000URLs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/docs/validate-public-docs.ts` around lines 244 - 306, Add focused regression tests for the public-document validation logic, covering wrapped MCP text detected by WRAPPED_RULES, missing deploy requirements reported by collectCoverageIssues, and pages missing the http://veryfront.me:3000 development URL. Use controlled fixture content and assert the resulting issue paths, messages, and line/text details.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/getting-started/create-project.md`:
- Around line 106-109: Specify the text language on all three CLI output fences
by changing their opening fences to text: docs/getting-started/create-project.md
lines 106-109 and 126-131, and docs/getting-started/quickstart.md lines 82-85.
In `@docs/guides/coding-agents.md`:
- Line 199: Update the MCP port instructions at the earlier references around
lines 108-109 and 212 to derive the endpoint from the actual port in the dev
server’s printed URL, adding two to that bound port. Remove wording that
calculates from the requested --port while preserving the existing MCP URL
format.
In `@docs/guides/project-structure.md`:
- Line 188: Update the cache-location wording in the project structure guide to
state directly that development writes .cache/ in the project root and
production does not. Use concise, active, present-tense language and remove the
ambiguous phrase.
- Around line 236-264: Remove the duplicate “Generated directories” section
beginning at the later heading, including its directory table and cache
guidance. Preserve the earlier “Generated directories” section and its complete
production, cache-root, and environment-variable documentation.
---
Nitpick comments:
In `@scripts/docs/validate-public-docs.ts`:
- Around line 244-306: Add focused regression tests for the public-document
validation logic, covering wrapped MCP text detected by WRAPPED_RULES, missing
deploy requirements reported by collectCoverageIssues, and pages missing the
http://veryfront.me:3000 development URL. Use controlled fixture content and
assert the resulting issue paths, messages, and line/text details.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7335d58b-84dc-4034-9699-2adfaf12327c
📒 Files selected for processing (10)
deno.jsondocs/getting-started/create-project.mddocs/getting-started/deploy-project.mddocs/getting-started/installation.mddocs/getting-started/quickstart.mddocs/guides/coding-agents.mddocs/guides/deploying.mddocs/guides/project-structure.mddocs/guides/providers.mdscripts/docs/validate-public-docs.ts
The backfill replayed a "Generated directories" section from the docs mirror into project-structure.md, but this repo already had a newer one: #3606 landed the production cache root and VF_CACHE_DIR here after the mirror copy was written. The result was two H2 sections with the same heading giving different answers, the later one claiming the CLI writes .cache/ into the project root with no mention of production. Drop the stale copy; the retained section is a strict superset of it. Also rewords the summary line above that section: "`.cache/` is too during development" elided its verb across a sentence boundary. coding-agents.md still stated the MCP port as `--port + 2` in two places. The earlier fix only corrected the verification section, and one of the two misses was the "Port already in use" heading, which is precisely the case where the requested port and the bound port differ. mcpPort is computed from started.port (cli/commands/dev/command.ts:240-243), so a --port 4000 that falls forward to 4001 serves MCP on 4003.
CI: three red checks, one cause, and it is not this PRThe three failures collapse to a single shard flake that this branch cannot reach.
That shard passed every test it ran and then died on the way out:
Why it is not this branchThis PR touches 10 files: 8 markdown pages, Shard 4's file list includes So: pre-existing flake, already fixed, fix already queued. I have not touched that test. StatusThe review-thread fixes in 4acf390 triggered a fresh run, which supersedes the failed jobs. If shard 4 flakes again before #3631 lands, it is the same leak and wants a re-run rather than a change here. The full pre-push suite (fmt check + tests) passed locally on 4acf390 before this was pushed. |
kwakayama
left a comment
There was a problem hiding this comment.
Findings
-
[Medium]
docs/getting-started/installation.md:101makes the new “Node.js, Deno, or Bun” manual-install path runnable only with npm. A Deno-only or Bun-only user can follow the showndeno add npm:veryfront/bun add veryfrontsetup, then immediately fail becausenpmneed not be installed. The supported Deno scaffold instead writesdeno run -A npm:veryfront@<version> dev(cli/commands/init/deno-config-generator.ts:8-14). Provide runtime-specific run commands (and the required Deno task/config). -
[Medium]
docs/guides/coding-agents.md:108-109says the HTTP MCP server listens “two ports above” the bound dev port, while:199diagnoses connection refusal only as a stopped/non-default dev server. This is not guaranteed:cli/commands/dev/command.ts:242-248deliberately continues when MCP startup fails, andcli/mcp/server.ts:224-235disables MCP when that port cannot bind. A local process onboundPort + 2therefore leaves the documented endpoint refused despite a healthy dev server. Document the collision case/actual availability, or implement and report an MCP fallback port.
| Rubric | Score |
|---|---|
| Correctness | 31/40 |
| Tests | 16/20 |
| Reliability/security | 12/15 |
| Maintainability | 14/15 |
| Scope/docs | 7/10 |
| Total | 80/100 |
Review-Gate:
Reviewer: Codex
Reviewed-SHA: c7b48f5
Score: 80/100
Actionable-Findings: 2
Verdict: REQUEST_CHANGES
The deploy pages said a protected environment serves only a signed-in browser and that every other request sees the sign-in redirect. The second half is wrong. checkProtectedProxyAccess reads the session out of an `authToken` cookie (extractUserToken, src/proxy/proxy-token-resolution.ts:52) and never inspects the client, so any HTTP client that sends that cookie is served. `veryfront deploy` relies on it: its readiness probe sets `Cookie: authToken=<token>` when the stored credential is a session token (deploy-project.ts:969), and withholds an opaque API key because extractUserIdFromToken resolves no userId from one. The genuinely true part is kept and narrowed to what the code does: an API key does not open deployment traffic, because the gate has no API-key branch and answers such a request with the same 302 an anonymous one gets. Also records that Deploy treats a sign-in redirect, a 401, and a 403 alike as the protection challenge (deploy-project.ts:989), which is what lets a keyless probe pass. Pins the corrected contract in tests/docs/guide-content.test.ts; the new case fails against both pages as they read before this change.
|
Re: the P2 on the protected-environment access contract (review). Fixed in 0d5fbdf. What the source actually says I read the gate and the client before writing, and the axis is the credential, not the client:
What changed
Two smaller edits on the same page for coherence: the section opener now says a protected environment "serves only requests that carry a Veryfront user session for a member of the project", with the cookie named and the browser described as how a browser gets one, instead of "serves only that signed-in member"; and the Verify-it-worked remedy now offers repeating the request with a member's session in an
No em dashes added; the new prose uses commas, colons, and separate sentences. Tests No test asserted the old wording, so nothing had to be relaxed. I added a case instead: Gates
Not re-queued: leaving that to @kwakayama. |
Why
The veryfront-docs Update Code Docs workflow has failed on every run since 2026-05-27. It fails at Validate synced code docs quality with 13 issues, so no documentation change made in this repo has reached veryfront.com in ~2.5 months. Latest failing run: 31564458030.
The validator runs against veryfront-docs'
docs/code/**after the sync overwrites that tree from this repo'sdocs/. The failures are ours, so the fix has to land here. Fixing veryfront-docs alone unblocks nothing.1. The 13 validator failures
Each fix was verified against the source, not assumed:
veryfront devprints✓ Ready in <duration>then the URL on its own line, notReady on <url>cli/commands/dev/command.ts:329/mcp; the dev server lists it only under--verbosecli/commands/dev/command.ts:240-243Protected · Release <version>cli/commands/deploy/command.ts:171302tohttps://<apex>/sign-in?from=...src/proxy/proxy-access-control.ts:218deploying.mdDeploy pages now state that Cloud environments are protected by default, that an unauthenticated request is redirected to sign-in, that
VERYFRONT_API_TOKENdoes not open a protected environment, and they name the Public Environment Studio switch.I also replaced the
curl -sSf <environment-url>verification on the deploy page. Against a protected environment that only ever proved the sign-in redirect, so it never verified what it claimed to.2. The backfill (this is the part that would have destroyed shipped work)
While the sync was broken, 20 docs PRs were merged directly into veryfront-docs to get fixes onto the live site. Those edits exist only in the mirror. The first green sync run would have silently reverted every one of them.
Rather than trust the PR list, I replayed the sync locally (copy
docs/into a checkout of veryfront-docsorigin/main, apply the workflow's link-rewrite transform) and diffed the result against the live mirror. That isolates mirror-only content exactly. Note the real drift starts at #357, not #366 — the last successful content sync wasc10037f(2026-08-02).Backfilled:
tsconfig.jsonandreact/react-domas dependencies:scripts/build/build-npm-dnt.ts:313,337..cache/anddist/generated-directory reference, the self-ignoring.cache/.gitignore, andVERYFRONT_CACHE_DIR(src/utils/cache-dir.ts:154).ext-llm-openaireference. The relative path escaped the docs tree and 404s once published.Deliberately not backfilled, because this repo is genuinely ahead and the sync should overwrite the mirror:
open --site(#3605),install --target(#3558),npx veryfront@latest,--output/outDir(#3565),tasks//schedules//webhooks//evals/discovery, and the skills-authorization andallowed-toolssemantics inagents.md. I checked each against the CLI source rather than assuming the newer commit date wins.3. Why it broke silently, and why it will not again
scripts/docs/validate-public-docs.tsalready had the em-dash rule — and the em dash still reached main. It only runs underdeno task docs:validate, which CI never invokes; CI's lint shard runslint:ci. The accuracy contract was enforced only downstream, in another repo, on arepository_dispatchrun nobody watches.sync-docs.ymlfires on every docs push regardless of validity.So this PR mirrors the deploy-access, dev-server-URL, and MCP-endpoint coverage checks into the local validator and wires it into
lint:ciasdocs:public:check. A PR that breaks the sync now fails here, in the repo that caused it.No validator rule was weakened, skipped, or removed. The change is purely additive.
Verification
I reproduced the pipeline rather than eyeballing the docs.
sync-code-navigation.mjs) and ran veryfront-docs' unmodifiedcheck-code-docs-quality.mjsagainst the result:13 code docs quality issue(s) found— byte-identical to the CI failureValidated code docs quality across 113 file(s).check-legacy-identity-docs.mjsalso passes.deno task lint:ci— passes, withdocs:public:checkrunning inside it.deno task docs:validate— passes, 1238 doc links OK.deno task fmt:check,docs:api-reference:check,docs:errors:check— pass.The gate has already earned its keep
While this PR was open, #3616 landed overlapping deploy-access work on
deploy-project.md— carrying two em dashes. Merging main here, the newdocs:public:checkfailed immediately:Without this gate that content would have reached main and broken the docs sync again, on the same rule, days after it was repaired. Fixed in the merge commit.
On the merge itself I dropped my
## Environment accesssection in favour of #3616's. Theirs is better researched (it covers the403non-member case and the fact that the sign-in apex varies by host) and is pinned by the regression test that PR added.tests/docs/guide-content.test.tspasses, all 33 contracts. The#environment-accessanchor my other pages link to still resolves, since both versions use the same heading.Companion PR
None needed. Once this merges, the sync's next run regenerates
docs/code/**from this branch's content and reconciles the mirror on its own. A veryfront-docs PR would only duplicate what the pipeline is now able to do.Summary by CodeRabbit
Documentation
Chores