Skip to content

feat(rendering): bind prepared imports to render generations - #4458

Merged
kojiwakayama merged 3 commits into
mainfrom
feat/prepared-render-module-loader
Sep 8, 2026
Merged

kojiwakayama merged 3 commits into
mainfrom
feat/prepared-render-module-loader

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Next bounded executor-side prerequisite for https://github.com/veryfront/veryfront-issue-inbox/issues/1035, building on #4456.

  • Add an internal createPreparedRenderModuleLoader that captures one complete generation binding and bounded source/package import tables before asynchronous hashing.
  • Reuse RuntimeModuleLoader, canonical project-path validation, registered errors, and the existing generation identity. Unknown imports fail without cache, filesystem, package, or network fallback. Import callbacks remain lazy; native imports own module caching.
  • Replace the ad hoc dispatcher in the existing native page-generation fixture with this loader. Its trusted test parent hashes its known source files/configuration and passes the actual prepared artifact identity; the child verifies the derived identity before serving the unchanged SSR handler.
  • Keep artifact ownership, process shutdown, and admission in the existing generation lifecycle. No new dependency, public export, issuer, allocator, cache, queue, or deployment flag.

This four-file change is mostly focused tests and fixture wiring. The production implementation is one cohesive import capability.

Trust boundary

The loader captures import callbacks, not the entire project filesystem. Trusted bootstrap must verify its compiler-generated tables against the artifact identity and retain the matching immutable source view. This change does not perform authorization, verify source bytes in production, grant execution authority, or provide isolation. A host must not rebind a realm that has executed one tenant to another tenant.

No production dispatch, shared credential transport, or Cloud activation is introduced. Source capture/verification, scoped renderer authority, isolated bootstrap/dispatch, native package support, and strict cold-replica/rolling-replacement hydration verification remain in #1035/#1042.

Verification

  • 67 focused unit cases and 9 native process-lifecycle cases pass on Deno, Node, and Bun, including the complete page-rendering fixture, concurrent request isolation, lazy imports, cache removal, and executor exit before artifact cleanup.
  • Additional prepared-module, custom error-page, named-layout, and RSC unit cases pass.
  • Explicit Deno typechecks, deno task lint:ci, deno task fmt:check, and diff checks pass.
  • Both codex review --uncommitted and codex review --base origin/main completed without actionable findings for head ae5b17353f324f9e2f01b988a2aeaf8cf783d662. The branch review reproduced all 67 focused unit cases and passed explicit type/diff checks. Its sandbox cannot open loopback listeners; native integration results above were verified in the normal environment on all three runtimes.

No production promotion or staging activation is part of this PR.

Summary by CodeRabbit

  • Reliability

    • Rendering now uses a prepared, validated module-loading process for each generation.
    • Source and package modules remain isolated, with clearer handling for missing, malformed, or invalid module references.
    • Import tables are bounded to help prevent oversized or unsafe render configurations.
    • Render generation identity is validated consistently across the rendering pipeline.
  • Tests

    • Added coverage for deferred loading, validation, namespace isolation, error handling, and generation-specific module resolution.

@greptile-apps greptile-apps 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.

Your trial has ended. Reactivate Greptile to resume code reviews.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 7a7d5138-a966-4ad4-9479-90984e56bd78

📥 Commits

Reviewing files that changed from the base of the PR and between 9c3c32c and e5f2afc.

📒 Files selected for processing (4)
  • src/rendering/prepared-module-loader.test.ts
  • src/rendering/prepared-module-loader.ts
  • tests/integration/renderer/fixtures/generation-page-executor.ts
  • tests/integration/renderer/render-generation.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a validated prepared render module loader. It captures source and package imports, resolves generation identity, validates module references, and integrates serialized binding data into renderer pipeline tests.

Changes

Prepared render module loading

Layer / File(s) Summary
Loader contract and input validation
src/rendering/prepared-module-loader.ts, src/rendering/prepared-module-loader.test.ts
The loader defines its options and result interfaces, validates project paths, tables, callbacks, references, and entry limits, and captures immutable import tables.
Lazy import dispatch and validation
src/rendering/prepared-module-loader.ts, src/rendering/prepared-module-loader.test.ts
The loader resolves source and package references on request, preserves callback results and failures, rejects invalid namespaces, and exposes generation identity.
Generation binding integration
tests/integration/renderer/...
The pipeline test serializes generation binding metadata. The page executor creates the prepared loader, verifies identity, and delegates runtime imports to it.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to e5f2a

The prepared loader and fixture integration preserve the validated generation-binding and module-import contracts without an identified merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant RenderGenerationTest
  participant GenerationPageExecutor
  participant PreparedRenderModuleLoader
  participant RuntimeModuleLoader
  RenderGenerationTest->>GenerationPageExecutor: pass serialized binding data
  GenerationPageExecutor->>PreparedRenderModuleLoader: create loader
  PreparedRenderModuleLoader-->>GenerationPageExecutor: return identity-bearing loader
  RuntimeModuleLoader->>PreparedRenderModuleLoader: import module reference
  PreparedRenderModuleLoader-->>RuntimeModuleLoader: return prepared namespace or error
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: it binds prepared imports to render generations through the new prepared module loader.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/prepared-render-module-loader

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 289 2303 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

Copy link
Copy Markdown
Contributor

Review: 92/100 — Excellent

Solid, tightly-scoped internal change that fits the codebase's existing security-hardened idioms very well.

Strengths

  • Defensive-by-construction: consistently uses Object.create(null) for captured tables (prevents __proto__/constructor collisions even though canonical-path validation would technically allow a segment named __proto__), own-data-property checks (getOwnPropertyDescriptor + hasOwn(..., "value")) to avoid triggering getters/proxy traps, and no-hook proxy detection (isProxyWithoutHooks) before touching any caller-supplied object — matches the pattern already established in render-generation-binding.ts.
  • Correct trust-boundary framing: capture happens synchronously before any await, so mutating the input after calling createPreparedRenderModuleLoader can't affect the frozen result (verified by the "captures binding, root and callback tables before yielding" test) — an easy bug to introduce in an async factory and it's handled correctly.
  • Test coverage is thorough: 283 lines covering hook-avoidance (asserts hooks === 0 for proxies/getters), prototype-pollution-adjacent inputs, malformed/oversized specifiers, entry-budget enforcement across the combined source+package table, exact error-slug assertions, and cross-replica identity stability. This is well above the norm for a 164-line implementation file.
  • Clean fixture migration: generation-page-executor.ts now defers entirely to the new loader instead of hand-rolling table lookups, and the identity check added there (loader.identity.scopeId/generationId vs. the host's expected binding) is a meaningful integration assertion, not just plumbing.
  • Scope discipline: no new public export (src/rendering/index.ts untouched), no dependency, cache, or queue added — matches the PR description's stated boundaries, and the doc comments are honest about what this doesn't do (no source-byte verification, no isolation, no authorization).

Minor nits (non-blocking)

  • CHANGELOG.md isn't updated, which CONTRIBUTING.md technically calls for on feat: commits — but this matches the precedent set by the direct predecessor PR (feat(rendering): derive complete render generation identities #4456, same author/pattern), so I'd leave it to maintainer discretion rather than block on it.
  • specifier() in prepared-module-loader.ts is really a generic "bounded, well-formed string" validator (it's used for projectDir, table keys, and package specifiers alike) — the name reads as package-specifier-specific. A name like boundedString() would reduce a moment of confusion on re-read.
  • The factory doc comment mentions "source and artifact bytes are bounded separately," but this file only bounds table entry count, not byte sizes of anything — presumably referencing the follow-up work in feat(extensions): extension loader with topo sort and lifecycle #1035/Bump veryfront-code to 0.1.203 #1042, but as written it could be misread as something this file already does.

Verification note: deno isn't available in this review sandbox, so I could not independently re-run the 67 unit / 9 integration cases the PR description claims. I traced the logic against the test file by hand instead (including the hook-avoidance and prototype-safety cases) and didn't find a mismatch between test expectations and implementation.

No blocking issues found. Nice work.


Generated by Claude Code

@gitar-bot

gitar-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review ✅ Approved

Binds prepared imports to render generations by introducing an internal createPreparedRenderModuleLoader that captures generation bindings and bounded import tables before asynchronous hashing. Reuses existing RuntimeModuleLoader infrastructure and generation identity; unknown imports fail without fallback. Test fixture updated to verify derived artifact identity. All 67 focused unit cases and 9 native process-lifecycle cases pass across Deno, Node, and Bun. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@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: ae5b17353f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/rendering/prepared-module-loader.ts Outdated

@greptile-apps greptile-apps 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.

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

@codex review

@greptile-apps greptile-apps 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.

Your trial has ended. Reactivate Greptile to resume code reviews.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Fixed the coverage crash in e5f2afc. Deno 2.7.7 crashes while serializing coverage when the malformed Unicode source key becomes an inferred callback name. The invalid source key remains covered; the fixture now references a normally named callback.

Reproduced the CI panic locally with deno task test:file src/rendering/prepared-module-loader.test.ts --coverage=<COVERAGE_DIR> --coverage-raw-data-only before the change. The same command now passes all 11 steps and writes coverage successfully. Explicit type checking, formatting, lint, and diff checks also pass.

The import-path review comment is fixed and resolved. CI and a fresh Codex review must pass on this head.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: e5f2afc3ea

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sonarqubecloud

sonarqubecloud Bot commented Sep 8, 2026

Copy link
Copy Markdown

@kojiwakayama
kojiwakayama added this pull request to the merge queue Sep 8, 2026
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Ready for review on e5f2afc: all checks pass, all review threads are resolved, and the branch has no merge conflicts. Codex passed this exact commit: #4458 (comment)

The import alias and byte-limit documentation are corrected. The malformed-Unicode rejection test remains in place with a callback name that Deno can serialize for coverage. A separate intermittent WebSocket cleanup failure passed after retrying the unchanged head; its focused coverage test also passed 10 consecutive local runs.

Merged via the queue into main with commit 71df458 Sep 8, 2026
101 of 105 checks passed
@kojiwakayama
kojiwakayama deleted the feat/prepared-render-module-loader branch September 8, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants