Skip to content

fix(event): keep event.context and req.context as one reference#1499

Merged
pi0 merged 1 commit into
mainfrom
fix/event-context-req-context-ref
Jul 22, 2026
Merged

fix(event): keep event.context and req.context as one reference#1499
pi0 merged 1 commit into
mainfrom
fix/event-context-req-context-ref

Conversation

@pi0x

@pi0x pi0x commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Problem

getRequestIP silently ignores event.context.clientAddress. Writing to the documented type surface — clientAddress is declared on H3EventContext (src/types/context.ts:27) — never reaches the reader at src/utils/request.ts:542, which reads event.req.context?.clientAddress. The call falls through to event.req.ip instead.

app.get("/", (event) => {
  event.context.clientAddress = "9.9.9.9";
  return getRequestIP(event); // undefined (expected "9.9.9.9")
});

Root cause

The invariant event.context === event.req.context is supposed to hold, but the constructor never established it. Two adjacent PRs conspired:

They only coincide when the runtime pre-populated req.context — and srvx does not do so by default, so the common path diverges. The ?. masked the breakage as a silent undefined.

Fix

Assign the resolved context back to req.context so both point at one object across all three construction paths (explicit context, inherited req.context, fresh fallback):

this.context = req.context = context || req.context || new EmptyObject();

Tests

Added regression tests in test/unit/event.test.ts:

  • event.context === event.req.context when neither is provided
  • same reference when an explicit context is passed
  • pre-populated req.context is reused
  • getRequestIP observes clientAddress written to event.context

All three failing-before assertions pass after the fix; full suite (1581 tests) + lint/typecheck green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Ensured request and event context consistently reference the same object.
    • Preserved existing request context or applied the provided context when available.
    • Improved client IP retrieval when the address is stored in the event context.
  • Tests

    • Added coverage for context sharing, reuse, and client IP behavior.

The H3Event constructor resolved `context` from `context || req.context ||
new EmptyObject()` but only assigned the result to `event.context`, never
writing it back to `req.context`. The two objects therefore diverged whenever
an explicit context was passed or `req.context` was unset (the default path,
since srvx does not populate `req.context`).

This silently broke `getRequestIP`, which reads `event.req.context?.clientAddress`:
a value written to the documented surface `event.context.clientAddress`
(H3EventContext, where the field is declared) never reached the reader and the
call fell through to `event.req.ip`.

Assign the resolved context back to `req.context` so `event.context ===
event.req.context` holds across all construction paths.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@pi0x
pi0x requested a review from pi0 as a code owner July 22, 2026 11:09
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

H3Event now keeps event.context and req.context synchronized, with tests covering fallback, provided, and existing request contexts plus client address lookup.

Changes

Context synchronization

Layer / File(s) Summary
Synchronize context and validate lookups
src/event.ts, test/unit/event.test.ts
The constructor assigns the resolved context to both event and request objects, and tests verify shared references, reused request context, fallback context, and getRequestIP behavior.

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

Suggested reviewers: pi0, pi0

Poem

I’m a bunny with context, aligned ear to ear,
Event and request now share what they hold dear.
A client address hops into view,
Tests check the references too.
One tidy change—thump, hooray!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fix: making event.context and req.context share the same reference.
✨ 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/event-context-req-context-ref

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.

@pi0 pi0 changed the title fix(event): keep event.context and req.context as one reference fix(event): keep event.context and req.context as one reference Jul 22, 2026
@pi0
pi0 merged commit b3c3eff into main Jul 22, 2026
9 checks passed
@pi0
pi0 deleted the fix/event-context-req-context-ref branch July 22, 2026 11:15
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