From 7dfb1f8cf995c5252d0573528d45a43dce697164 Mon Sep 17 00:00:00 2001 From: Maurice Schmicking <17197791+mschmicking@users.noreply.github.com> Date: Sat, 8 Aug 2026 11:20:08 +0200 Subject: [PATCH] docs: add AGENTS.md with contributor and agent guidance Captures the conventions that are not discoverable from the code: that vendor/ is a verbatim upstream copy and must not be patched, that argument validation goes through CheckArgs and Lua errors through ThrowLuaError, that relative stack indices must be resolved before pushing, and that unprotected Lua errors abort the process rather than throwing. Also records the workflow rules that caused real friction: branch from the default branch rather than stacking, since merges are squash-only; PR titles drive the release version; and CodeQL results must be read from a default-branch analysis, because pull-request analyses only report alerts new relative to the base and so read as clean when they are not. Contains no personal information. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..0b3aff2 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,124 @@ +# Working on this repository + +Guidance for AI agents and new contributors. Read this before changing anything. + +## What this is + +A Node.js native addon embedding Lua 5.1. The binding in `src/` is C++ against +[Node-API](https://nodejs.org/api/n-api.html); Lua 5.1.5 and LuaFileSystem are vendored under +`vendor/` and compiled in. There is no system Lua dependency and no prebuilt binary — every +install compiles from source. + +## Build and test + +``` +npm install # compiles the addon (runs node-gyp via the install script) +npm test # node --test +npx node-gyp rebuild # rebuild without reinstalling +``` + +After changing anything in `src/`, rebuild before testing — `npm test` runs against the last +compiled `build/Release/nodelua.node` and will happily pass on stale output. + +Also run the examples; they exercise paths the tests do not: + +``` +node examples/simple/index.js +node examples/lua_require/index.js +node examples/lua_lfs/index.js +``` + +## Layout + +| Path | Notes | +|---|---| +| `src/` | The binding. This is what you change. | +| `vendor/lua/` | Lua 5.1.5, verbatim upstream — **do not modify** | +| `vendor/lfs/` | LuaFileSystem 1.8.0, verbatim upstream — **do not modify** | +| `test/` | Suite and fixtures | +| `docs/` | Reference documentation | +| `binding.gyp` | Build definition, all platforms | + +`vendor/` is deliberately an unmodified copy so upgrading Lua stays a file copy rather than a +merge. If something in there needs fixing, fix it upstream or work around it in `src/`. + +## Conventions in `src/` + +- **Tabs for indentation**, matching the existing files. +- **Argument validation goes through `CheckArgs`**, not hand-written checks: + ```cpp + Napi::Value LuaState::SetField(const Napi::CallbackInfo& info) { + Napi::Env env = info.Env(); + if(!CheckArgs(info, "SetField", {Arg::Number, Arg::String, Arg::Any}) || !EnsureOpen(env)){ + return env.Undefined(); + } + ... + ``` + It generates the error messages, so the wording stays consistent. The two exceptions are + `Pop` and `SetTop`, whose argument is optional. +- **Errors from Lua go through `ThrowLuaError`**, which builds the message, **pops the error** + and throws. Popping by hand at the call site is how the Lua stack silently grows. +- **Every method that touches `lua_` must call `EnsureOpen` first.** Without it, use after + `Close()` is a use-after-free rather than an exception. +- **Resolve relative stack indices with `abs_index` before pushing anything.** Pushing shifts + every negative index by one. This was a real bug: `SetField(-1, ...)` ended up assigning into + the value it had just pushed. +- **Never let an unprotected Lua error reach the C API.** It aborts the whole process instead of + throwing. Guard the type first, as `SetField` and `GetField` do with `EnsureIndexable`. +- **Do not throw C++ exceptions through Lua frames.** The addon builds with + `NAPI_DISABLE_CPP_EXCEPTIONS` because Lua unwinds with `longjmp`, which would skip C++ + destructors. Use `ThrowAsJavaScriptException()` and return. + +## Tests + +`node:test` and `node:assert`, no test framework dependency — keep it that way. + +Every bug fix gets a regression test that fails before the fix. The suite already pins the +historic ones (argument order, boolean conversion, load-without-executing); if you change +behaviour, update the corresponding test deliberately rather than making it pass. + +Error message wording is asserted in tests. Changing it is a breaking change for anyone +matching on it. + +## Branches and pull requests + +- **Branch from the default branch.** Never base a branch on another open PR's branch — merges + here are squash-only, so a squashed parent leaves duplicated commits and guaranteed conflicts. +- **PR titles must be Conventional Commits.** The title becomes the squash commit and the release + version is derived from it, so a title that does not parse means a release that silently does + not happen. Allowed scopes are listed in `.github/workflows/pr-title.yml`. +- `feat` → minor, `fix`/`perf`/`refactor` → patch, `feat!` or a `BREAKING CHANGE` footer → major. +- All checks must pass, and the branch must be up to date with the default branch. If GitHub says + the branch is behind, that is not a conflict — use "Update branch". + +## CI + +Every PR builds and tests on Linux, macOS and Windows across three Node majors, and runs CodeQL +over the C++ and the JavaScript. + +CodeQL raises alerts inside `vendor/` because its path filters do not apply to compiled +languages. Those belong upstream — dismiss them rather than patching the vendored sources. + +When reading CodeQL results, check an analysis on the **default branch**. Pull-request analyses +only report alerts new relative to the base, so `results=0` on a PR does not mean the codebase is +clean. + +## Releases + +Do not bump the version or tag by hand. release-please maintains a release PR from the merged +commit history; merging it tags the commit, publishes a GitHub Release, and that event publishes +to npm over OIDC. See [docs/development.md](docs/development.md). + +Anything that ships must be in the `files` allowlist in `package.json`. The release workflow +asserts the tarball contains all 29 vendored Lua sources — a tarball missing them is unbuildable +for everyone who installs it, and npm versions are immutable. + +## Deliberately out of scope + +Do not add these without discussion; each was considered and declined: + +- **Prebuilt binaries.** Users need a C++ toolchain, as they always have. +- **Async execution.** The API is synchronous throughout. +- **Thread support.** One `LuaState` per thread is not supported, and Lua's `os.date` uses + non-reentrant C library calls. +- **Patching `vendor/`.** See above.