fix: check and verify for plans#1634
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
|
We've been automatically notified and are looking into it. Push a new commit to re-trigger the review, or contact support@builder.io if this keeps happening. Error ID: |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
This incremental pass fixes the three issues from the prior review: the public validator now has top-level requiresAuth: false, the offline fallback no longer rejects the renderer-supported <Columns columns={[...]}/> form outright, and the validator size cap has been raised to cover the CLI’s packaged payload budget. I verified those fixes and resolved the stale review threads before continuing.
Risk assessment: High. This PR still adds a new public unauthenticated validation action and changes the plan local verify fallback semantics, so parity gaps between the offline lint and the renderer remain important.
The overall approach is still sound: verify now reaches the renderer’s real parser/schema when available, and the nested checklist/question-form checks added here do close a meaningful false-green case. Targeted specs also pass on the updated head (packages/core/src/cli/plan-local.spec.ts and templates/plan/actions/validate-local-plan-source.spec.ts).
New findings:
- 🟡 The offline fallback still does not recurse into top-level
<Columns columns={[...]}/>blocks, so nested invalid content there can still false-green. - 🟡 Nested
visual-questionsblocks are still skipped by the new recursive fallback lint even though top-levelVisualQuestionsare validated.
🧪 Browser testing: Skipped — PR only modifies CLI/server/docs/tests, no UI impact.
| // recursing through nested containers. This is still a subset of the renderer | ||
| // schema (which is why `verify` is the authority), but it closes the common | ||
| // nested checklist / question-form / missing-id case offline. | ||
| const NESTED_CONTAINER_TAGS: ReadonlyArray<{ tag: string; attr: string }> = [ |
There was a problem hiding this comment.
🟡 Fallback lint still skips nested blocks inside top-level Columns JSON syntax
The new recursive fallback only scans TabsBlock/Tabs, so a top-level <Columns columns={[{ id, label, blocks: [...] }]} /> block still never gets its nested checklist/question blocks walked. When renderer validation is unavailable, verify can therefore still false-green invalid nested content in the renderer-supported Columns JSON form.
Additional Info
`NESTED_CONTAINER_TAGS` only includes TabsBlock/Tabs here, while top-level Columns JSON syntax is supported elsewhere in this PR and by the renderer round-trip tests.
| } | ||
| return; | ||
| } | ||
| if (type === "question-form") { |
There was a problem hiding this comment.
🟡 Nested visual-questions blocks are not validated by the recursive fallback lint
This recursive path only validates nested blocks with type === "question-form", but the top-level lint already treats VisualQuestions as equivalent question content. If renderer validation is unavailable, nested type: "visual-questions" blocks can still pass offline even when required question fields like id or mode are missing.
Additional Info
Top-level lint uses `/(QuestionForm|VisualQuestions)\b/` in `lintQuestionFormShape`, but the nested traversal only checks `type === "question-form"` here.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🔴
Review Details
This incremental pass re-checks the latest PR head after the follow-up fixes. I verified the previously discussed auth and fallback changes are in place, and I reran the targeted specs for packages/core/src/cli/plan-local.spec.ts and templates/plan/actions/validate-local-plan-source.spec.ts; both still pass.
Risk assessment: High. The PR continues to expose a new unauthenticated validation endpoint and uses it to change plan local verify semantics, so request-boundary protections matter as much as schema correctness.
I also re-checked the two already-open fallback-lint parity comments. They still appear to be unresolved, so I did not repost them.
New finding:
- 🔴 The public
validate-local-plan-sourceendpoint enforces its byte cap only inside the action body, after the action router has already parsed the full JSON request. That means an anonymous caller can still force large-body allocation/parse work on this new public route before the 16 MiB guard runs.
The renderer-backed verification direction is still good overall, but this request-size enforcement gap should be closed before treating the new public validator as safe to ship.
🧪 Browser testing: Skipped — PR only modifies CLI/server/docs/tests, no UI impact.
| "The plan MDX folder: plan.mdx plus optional canvas.mdx, prototype.mdx, .plan-state.json, and assets/.", | ||
| ), | ||
| }), | ||
| http: { method: "POST" }, |
There was a problem hiding this comment.
🔴 Public validator enforces payload size only after the body is fully parsed
This unauthenticated POST only checks MAX_MDX_BYTES inside run(), but the core action router parses the full JSON body first via webReq.json() / readBody(event). A large anonymous request can therefore still force memory/CPU work on this public route before the 16 MiB guard runs, so the size limit needs to be enforced at the HTTP body-read boundary too.
Additional Info
Confirmed against packages/core/src/server/action-routes.ts lines 304-312; `readBodyWithSizeLimit` exists in packages/core/src/server/h3-helpers.ts but is not used by the generic action POST path.
There was a problem hiding this comment.
good thing to add, let me scope a maxBodySize for this action and maybe in the future other actions can reuse it
2 participants
verifyPOSTs the MDX folder to the Plan app's new, public, no-DBvalidate-local-plan-sourceaction, which runs the renderer's ownparsePlanMdxFolder+planContentSchema. Its verdict gatesverify'sok, and rejected plans surface the renderer's exact schema-path issue (e.g.blocks[1].data.tabs[0].blocks[0].data.items[0].id). When the endpoint is unavailable (older/unreachable Plan app),verifydegrades to the transport checks and warns that content was not validated, rather than hard-failing.checknow recurses into nestedtabsblock arrays so the common nested-checklist / question-form / missing-idcase is caught offline, and it no longer reportsvalidation: "passed". It reports a clearly-scopedlint-passedwith a note pointing toverifyfor authoritative validation.