Code review batch 2: medium-severity bugs, security hardening, memory bounds - #2247
Merged
Merged
Conversation
This was referenced Jun 9, 2026
Closed
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f0b1f473e
ℹ️ 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".
A hung resumeFn previously blocked shutdown forever; cap the wait at 30s and abandon any still-active resumes so the process can exit. Closes #2239.
Release the pending timer even when the race rejects or the generator is abandoned mid-await, not just on the happy path. Closes #2240.
Bind the active render session via AsyncLocalStorage so concurrent SSR renders each record modules to their own session instead of whichever started first. Falls back to the single-session heuristic only when unambiguous. Closes #2237.
Only set Access-Control-Allow-Origin on an OPTIONS preflight when the origin passed validation, instead of falling back to the configured origin (e.g. "*") for an origin we just rejected. (#2241)
Return a generic 'credentials are not configured' description instead of naming the missing client id/secret env vars, which can propagate to HTTP responses. (#2241)
Reject cross-project sources larger than 5MB (via Content-Length and a post-read guard) so a crafted ref can't stream an arbitrarily large body into memory. (#2241)
Sessions stored in an unbounded Set never expired, so clients that disconnected uncleanly leaked entries forever. Track last-seen timestamps with a 30-minute inactivity TTL and lazy pruning (no background timer). Closes #2235 (session portion).
manifestStore grew per project:route and pendingCollections leaked on render errors. Cap both with LRUCache; eviction only drops a regenerable preload optimization. Closes #2235 (manifest portion).
Replace per-line split/map/join with code.replace(/^/gm, ' ') to avoid allocating an intermediate line array per module on the batch hot path. Behavior-identical. (#2242)
The second code-review batch bounds cross-project module fetches at 5MB. The fallback check needs to compare UTF-8 bytes rather than UTF-16 code units so sources without Content-Length cannot exceed the intended byte cap with non-ASCII text. This also prepares the rebased batch for release 0.1.740.\n\nConstraint: PR #2247 was rebased onto release 0.1.739 after #2244 merged.\nConstraint: veryfront-code release PRs must keep deno.json and VERSION in sync.\nRejected: Keeping deno.lock churn from local Deno verification | dependency-resolution metadata changed without a dependency update.\nConfidence: high\nScope-risk: narrow\nTested: deno test --frozen --allow-all src/modules/react-loader/ssr-module-loader/cross-project-import-loader.test.ts src/mcp/session.test.ts src/provider/runtime-loader.test.ts\nTested: deno check --frozen focused touched module set\nTested: deno task verify:quick\nTested: git diff --check
kojiwakayama
force-pushed
the
fix/code-review-batch2-2026-06
branch
from
June 9, 2026 21:34
0f0b1f4 to
c2c8395
Compare
The PR still had two review risks: expired MCP sessions could make HTTP transport skip session enforcement because the gate depended on active session count, and cross-project module fetches without Content-Length still buffered the whole body before applying the 5MB cap. Session enforcement now tracks whether a session header is required separately from the active-session map, preserving the explicit DELETE reset path while keeping expired sessions from reopening stateless handling. Cross-project source reads now share a streaming byte-counted helper across SSR import loading and module-server serving. Constraint: Explicit DELETE of the last session intentionally resets the transport to pre-init behavior. Constraint: Cross-project module fetches must keep the existing 5MB cap while avoiding unbounded buffering when Content-Length is absent. Rejected: Continue using sessionManager.size as the transport gate | lazy TTL pruning makes expired sessions look like no session history. Rejected: Keep response.text() plus byte-length fallback | it still buffers an unbounded response before rejecting. Confidence: high Scope-risk: moderate Tested: deno fmt --check targeted files Tested: deno lint targeted files Tested: deno check focused MCP and cross-project module files Tested: deno test --no-check --allow-all src/mcp/session.test.ts src/mcp/http-transport.test.ts src/mcp/server.test.ts src/modules/react-loader/ssr-module-loader/cross-project-import-loader.test.ts src/modules/server/module-server.test.ts Tested: git diff --check Not-tested: deno task verify:quick still fails on pre-existing broken doc links in docs/architecture/21-agent-tool-registration-current-state.md
This was referenced Jun 9, 2026
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
1 participant
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.
Second batch from the deep code review — the medium-severity items that aren't in #2244. Rebases onto current main and prepares release 0.1.740. Each commit is scoped to one issue or review tightening. Local pre-push passed formatting, lint, typecheck, and the full unit suite.
Bugs
stop()now bounds its wait (30s) instead of hanging forever on a stuckresumeFn.withToolInputStatusTransitionsclears its pending timeout infinally(covers race rejection and generator abandonment).AsyncLocalStorage, so concurrent SSR renders record modules to their own session instead of whichever started first. Safe single-session fallback for unwrapped callers.Security hardening (#2241, subset)
*).Memory bounds (#2235)
SessionManagergains a 30-min inactivity TTL with lazy pruning (orphan sessions from unclean disconnects no longer leak). Tests added.route-module-manifeststores (manifestStore,pendingCollections) bounded withLRUCache— they grew per project:route / leaked on render errors.Performance (#2242, subset)
replace(/^/gm, " ")instead of per-line split/map/join.Release
deno.jsonandVERSIONto0.1.740.Verification
deno task verify:quickdeno test --frozen --allow-all src/modules/react-loader/ssr-module-loader/cross-project-import-loader.test.ts src/mcp/session.test.ts src/provider/runtime-loader.test.tsdeno check --frozenon the focused touched module setDeliberately NOT changed (with rationale)
veryfrontTransformCache/frameworkFileCache): keyed by framework file path, so already bounded by the fixed framework file count — not actually unbounded.escapeHtml(...); not vulnerable. Other Low-severity security backlog (CSRF __Host- prefix, dev innerHTML, log injection, redirect re-check) #2243 lows remain backlog.