fix(vm): restore caller's open_upvalues after nested execution

.agents/plans/A39-require-leaks-open-upvalues.md

@@ -0,0 +1,185 @@
+---
+id: A39
+title: require() leaks inner module's open_upvalues into outer caller
+issue: 244
+pr: 245
+branch: fix/require-leaks-open-upvalues
+base: main
+status: review
+direction: A
+unlocks:
+  - luassert.assertions
+  - luassert.array
+  - luassert.spy
+---
+
+## Goal
+
+Fix `Lua.VM.Executor.execute/5` so that nested executions (most importantly
+`require`) no longer leak the inner module's `state.open_upvalues` map
+back to the outer caller. This unblocks loading real-world Lua libraries
+that mix nested `require` chains with many top-level `local function`
+definitions (luassert, busted, etc.).
+
+## Out of scope
+
+- Refactoring the upvalue / closure model. The bug is one missed
+  save/restore, not a design flaw.
+- Adding a "close all open upvalues at chunk end" sweep. Not needed once
+  save/restore is in place.
+- The full `package.searchers` mechanism. Unrelated.
+- Bytecode-encoder support for vararg chunks (chunks falling back to the
+  interpreter is what surfaces this bug, but the interpreter path should
+  be correct on its own).
+- Coordinating the `tv-labs/platform/sidecar` Lua bump. Mentioned in the
+  issue but a downstream concern.
+
+## Success criteria
+
+- [ ] `mix test` passes with no regressions vs. baseline (1772 tests).
+- [ ] New unit regression in `test/lua/vm/require_open_upvalue_test.exs`
+      reproduces the bug with a minimal two-file pure-Lua repro and
+      asserts the outer's local reads correctly.
+- [ ] New integration test under `test/integration/luassert_test.exs`
+      vendors a real subset of `luassert` + `say` and asserts that every
+      luassert module loads successfully via `require`.
+- [ ] `mix test --only lua53` suite count does not regress.
+
+## Implementation notes
+
+### Root cause
+
+`Lua.VM.Executor.execute/5` at `lib/lua/vm/executor.ex:73-82` resets
+`state.open_upvalues` to `%{}` at entry but never restores the caller's
+`open_upvalues` on return. Every other call site that descends into a
+nested execution (`call_function/3` for `:lua_closure`, `call_value/5`,
+the dispatcher entry, the dispatcher's frame returns, the interpreter's
+`:call` op for Lua closures) carefully saves the caller's map, resets
+to `%{}`, runs the callee, and restores on return. `Executor.execute/5`
+is the one outlier.
+
+When `require` is called as a `native_func` from a Lua execution, the
+inner module's `Lua.VM.execute(proto, state)` populates its own
+`open_upvalues` as closures are created over the inner module's
+top-level locals. When the inner returns, those entries leak back to the
+outer caller. If the outer then creates a closure that captures a
+top-level local at a register number that collides with one of the
+inner's leftover entries, the outer's closure **reuses the inner's
+stale cell**, aliasing the outer's local to whatever value the inner had
+at that register.
+
+For `luassert.assertions`, the outer's `assert` (reg 0) ends up aliased
+to the inner `luassert.assert` module's `s` (reg 0, the `say` module).
+At line 307, `assert:register(...)` reads `assert` through the stale
+upvalue cell and sees `say`, not the obj table — hence
+"attempt to call a nil value (method 'register' on local 'assert')".
+
+### Fix
+
+In `lib/lua/vm/executor.ex` `execute/5`, snapshot `state.open_upvalues`
+before resetting and restore it on the way out:
+
+```elixir
+def execute(instructions, registers, upvalues, proto, state) do
+  prev = Process.get(@position_key, @unset)
+  saved_open_upvalues = state.open_upvalues
+
+  try do
+    state = %{state | open_upvalues: %{}}
+
+    {results, regs, state} =
+      do_execute(instructions, registers, upvalues, proto, state, [], [], 0)
+
+    {results, regs, %{state | open_upvalues: saved_open_upvalues}}
+  after
+    restore_position(prev)
+  end
+end
+```
+
+Two callers of `Executor.execute/5`:
+
+- `Lua.VM.execute/2` (`lib/lua/vm.ex:26`) — used by
+  `parse_and_execute_module` (the bug path) and by top-level
+  `Lua.eval!`. Save/restore is correct in both cases.
+- `Lua.do_call_function/3` for `:lua_closure` (`lib/lua.ex:717`) —
+  called from the public `Lua.call_function/3`. Save/restore makes the
+  public API safer: callers don't lose `open_upvalues` across
+  `call_function` invocations.
+
+### Tests
+
+Two layers:
+
+1. **Unit regression** (`test/lua/vm/require_open_upvalue_test.exs`).
+   Minimal two-file pure-Lua repro: inner module declares a top-level
+   local at reg 0 and creates a closure capturing it; outer module
+   requires inner, then declares its own top-level local at reg 0 and
+   creates a closure capturing it. Assert the outer's local reads the
+   correct value, not the inner's leaked value.
+
+2. **Luassert integration** (`test/integration/luassert/` +
+   `test/integration/luassert_test.exs`). Vendor the luassert v1.9.0 +
+   say v1.4.1 source files under `test/integration/luassert/lua/`.
+   Assert that `require('luassert')` and every interior luassert module
+   load without error. Behavioural assertions (e.g.
+   `assert.are.equal(1, 1)` returns truthy) are deferred to a follow-up
+   plan; this PR proves the *loading* pipeline works.
+
+Vendor with upstream LICENSE files. Document the pin and source in
+`test/integration/luassert/README.md`.
+
+## Verification
+
+```bash
+mix format
+mix compile --warnings-as-errors
+mix test
+mix test test/lua/vm/require_open_upvalue_test.exs
+mix test test/integration/luassert_test.exs
+mix test --only lua53
+```
+
+Suite count before this plan: 1772 passing, 0 failing, 30 skipped.
+
+## Risks
+
+- **The fix changes observable state after a top-level `Lua.eval!`.**
+  Specifically, `state.open_upvalues` after an eval will now be the
+  pre-eval value (typically `%{}`) instead of whatever the chunk left
+  open. Mitigated by the full test run; `open_upvalues` is internal
+  state with no documented public consumers.
+- **`Lua.call_function/3` (public API) starts preserving the caller's
+  `open_upvalues`.** This is a behavior change, but the previous
+  behavior was the bug. Documented in `CHANGELOG.md`.
+- **Vendored luassert may shift if upstream changes.** Pinned to a
+  specific tag; future updates are explicit PRs.
+
+## What changed
+
+- **`lib/lua/vm/executor.ex`** — `execute/5` now snapshots
+  `state.open_upvalues` before resetting it to `%{}` and restores the
+  snapshot on return. Matches the save/restore pattern already used by
+  `call_function/3` for `:lua_closure`, `call_value/5`, and the
+  dispatcher entry.
+- **`test/lua/vm/require_open_upvalue_test.exs`** — new unit
+  regression. Two pure-Lua repros: (1) minimal inner-closure-shadows-
+  outer-local at reg 0 case; (2) the luassert-shape "many local
+  function defs between require and method call" that surfaces the
+  exact `attempt to call a nil value (method 'register' on local
+  'assert')` from #244.
+- **`test/integration/luassert/`** — vendored luassert v1.9.0 + say
+  v1.4.1 under `lua/`, with upstream LICENSE files alongside. README
+  documents the pin and update procedure.
+- **`test/integration/luassert_test.exs`** — 18 tests, no opt-in tag
+  so they run on every `mix test` (including CI). Without the fix, 8
+  of 18 fail (exactly the modules called out in #244 plus their API
+  smoke tests).
+- **`CHANGELOG.md`** — Unreleased / Fixed entry for #244 with the
+  side-effect note about `Lua.call_function/3`.
+
+### Suite delta
+
+- `mix test`: 1772 → 1792 (+20 new tests, 0 failures).
+- `mix test --only lua53`: 29 tests / 0 failures / 23 skipped
+  (unchanged).

CHANGELOG.md

@@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Fixed
+- `require` no longer leaks the loaded module's `open_upvalues` map back
+  to the calling chunk. Loading a module whose body created closures over
+  its own top-level locals could alias the caller's locals to stale inner
+  upvalue cells, breaking real-world libraries (e.g. `luassert.assertions`,
+  `luassert.array`, `luassert.spy`) that follow the pattern
+  `local x = require(...)` → many `local function` defs → `x:method(...)`.
+  As a side effect, `Lua.call_function/3` (public API) now preserves the
+  caller's `open_upvalues` across calls (#244).
+
 ## [v1.0.0-rc.0] - 2026-05-26
 
 This is the first release candidate for `1.0.0`. The library has been

lib/lua/vm/executor.ex

@@ -67,15 +67,28 @@ defmodule Lua.VM.Executor do
   Saves and restores any prior `current_position/0` snapshot so nested
   executions (e.g. an Elixir callback that itself calls `Lua.eval!`)
   don't leak source positions into each other.
+
+  Likewise saves and restores `state.open_upvalues` so that a nested
+  execution's upvalue cells — keyed by register index — cannot collide
+  with the caller's. Without this, a `require` that runs a module body
+  containing closures over its top-level locals would leak those cells
+  back to the caller; the caller's later closures would then reuse the
+  stale cells by register index, aliasing the caller's locals to
+  unrelated inner values.
   """
   @spec execute([tuple()], tuple(), list(), map(), State.t()) ::
           {list(), tuple(), State.t()}
   def execute(instructions, registers, upvalues, proto, state) do
     prev = Process.get(@position_key, @unset)
+    saved_open_upvalues = state.open_upvalues
 
     try do
       state = %{state | open_upvalues: %{}}
-      do_execute(instructions, registers, upvalues, proto, state, [], [], 0)
+
+      {results, regs, state} =
+        do_execute(instructions, registers, upvalues, proto, state, [], [], 0)
+
+      {results, regs, %{state | open_upvalues: saved_open_upvalues}}
     after
       restore_position(prev)
     end

test/integration/luassert/README.md

@@ -0,0 +1,86 @@
+# Vendored luassert integration test
+
+This directory contains vendored Lua source from two upstream libraries,
+used as end-to-end regression coverage for the `require` pipeline.
+
+## Pinned versions
+
+| Library  | Tag     | Upstream                                                |
+| -------- | ------- | ------------------------------------------------------- |
+| luassert | v1.9.0  | https://github.com/lunarmodules/luassert/tree/v1.9.0    |
+| say      | v1.4.1  | https://github.com/lunarmodules/say/tree/v1.4.1         |
+
+## Why these libraries
+
+luassert is the assertion library used by [busted](https://lunarmodules.github.io/busted/),
+the dominant testing framework in the Lua ecosystem. It exercises:
+
+- Multi-level `require` chains.
+- Modules with 50+ top-level `local function` definitions that close
+  over the module's own top-level locals.
+- Modules that return tables and modules that only register side
+  effects and return nothing.
+- `setmetatable`, `__call`, `__index` metamethods on returned values.
+
+`say` is luassert's i18n dependency. Both are pure Lua, no C bindings.
+
+The shape of `luassert/assertions.lua` — `local assert = require('luassert.assert')`,
+followed by many `local function` definitions, followed by
+`assert:register(...)` — is what surfaced the bug fixed in
+[#244](https://github.com/tv-labs/lua/issues/244).
+
+## Layout
+
+```
+lua/
+├── luassert/
+│   ├── LICENSE                  ← upstream MIT license
+│   ├── init.lua                 ← top-level entrypoint
+│   ├── assert.lua               ← core obj/metatable
+│   ├── assertions.lua           ← built-in assertions (the bug's epicenter)
+│   ├── modifiers.lua
+│   ├── array.lua
+│   ├── spy.lua / stub.lua / mock.lua
+│   ├── match.lua
+│   ├── state.lua / util.lua / namespaces.lua / compatibility.lua
+│   ├── formatters/
+│   ├── matchers/
+│   └── languages/
+└── say/
+    ├── LICENSE                  ← upstream MIT license
+    └── init.lua                 ← i18n string lookup
+```
+
+The `lua/` prefix matches the conventional `package.path` of
+`?.lua;?/init.lua`, so `require('luassert')` resolves to
+`lua/luassert/init.lua` and `require('luassert.assert')` resolves to
+`lua/luassert/assert.lua`.
+
+## Updating the pin
+
+```
+# From repo root.
+cd /tmp
+rm -rf luassert-* say-*
+curl -sL -o luassert.tar.gz \
+  https://github.com/lunarmodules/luassert/archive/refs/tags/vX.Y.Z.tar.gz
+curl -sL -o say.tar.gz \
+  https://github.com/lunarmodules/say/archive/refs/tags/vA.B.C.tar.gz
+tar -xzf luassert.tar.gz && tar -xzf say.tar.gz
+
+cd <repo>/test/integration/luassert/lua
+rm -rf luassert/ say/
+mkdir -p luassert say
+cp -r /tmp/luassert-X.Y.Z/src/* luassert/
+cp -r /tmp/say-A.B.C/src/say/init.lua say/
+cp /tmp/luassert-X.Y.Z/LICENSE luassert/LICENSE
+cp /tmp/say-A.B.C/LICENSE say/LICENSE
+```
+
+Then update the version table above and re-run
+`mix test test/integration/luassert_test.exs`.
+
+## License
+
+Both libraries are MIT-licensed. The upstream `LICENSE` files are
+preserved alongside the vendored source.

test/integration/luassert/lua/luassert/LICENSE

@@ -0,0 +1,22 @@
+MIT License Terms
+=================
+
+Copyright (c) 2012 Olivine Labs, LLC.
+
+Permission is hereby granted, free of charge, to any person obtaining a copy of 
+this software and associated documentation files (the "Software"), to deal in 
+the Software without restriction, including without limitation the rights to 
+use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies 
+of the Software, and to permit persons to whom the Software is furnished to do 
+so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all 
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR 
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, 
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE 
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER 
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, 
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE 
+SOFTWARE.