Repository navigation
feat: 1.0.0 — permission reasons, npm prebuilds, test coverage - #2
Merged
Merged
Conversation
Release prep for 1.0.0. The through-line is that everything here was
verified against a running system rather than reasoned about: 158 C++
tests and 58 Node tests, plus a packed tarball installed into an empty
project outside the repo.
Permission denials carry a reason (core, C, Python, Node)
A policy or approver could refuse a call but not say why, so a model
was told only that the call failed and retried variations of it --
each retry another prompt for a user who had already answered.
- PermissionVerdict{decision, reason}, implicitly constructible from
a bare PermissionDecision so existing policies keep compiling; this
is why none of the 11 pre-existing permission tests needed editing.
- The reason lands in the denial payload as "message", omitted rather
than sent empty so a consumer can tell "no reason" from "blank".
- Ask with no approver was the one denial with nobody to explain it,
so the SDK supplies a default.
- C API: the reason is a caller-provided AI_REASON_MAX buffer. An
earlier out-param draft was wrong -- the SDK read the pointer after
the callback returned, which dangles for any callback that points
at a local. A buffer has no lifetime question and no allocation.
- Node: a policy or approver may return {decision, reason}; a bare
number still works.
npm distribution
- binding.gyp, a prebuild script, a publish guard and a CI matrix for
linux/darwin x64/arm64. The C++ library links a static OpenSSL, so
the prebuild is self-contained; a dynamic one would either record
the build machine's path or put a second libssl into a Node process
that already statically links its own.
- install.cjs overrides npm's default node-gyp rebuild, which cannot
succeed on a user's machine, and checks for a source checkout before
a prebuild so a developer editing addon.cpp does not silently run a
stale binary.
Testing
- local_http_server now records request bodies, which is the only way
to see what the model was actually sent -- and therefore the only
proof that a reason survives the C API.
- New: permission reasons (6), C binding reason transfer including a
buffer filled exactly with no NUL (2), streaming, data stream,
base64, providers, MCP.
- The new assertions were checked against deliberately broken
implementations and fail as expected.
Not verified: the prebuilds workflow needs a push to run, and no CI has
ever built Windows.
Closing the acceptor does not cancel a blocking accept(). POSIX leaves that unspecified, and Linux -- unlike BSD -- does not wake the blocked thread: the blocked syscall holds a reference to the listening socket, so the close never reaches it. macOS passed while both Linux jobs hung for the full 1500s ctest timeout, in ~LocalHttpServer's join(), for all nine GatedAgent tests -- whose assertions had already passed by the time the destructor ran. Wake it with a connection of our own instead: the kernel completes the handshake into the backlog whether or not anyone is in accept(), so the thread returns, sees the stop flag and leaves. A thread parked in read_some() is reached with shutdown(), which does wake a blocked reader -- close() does not do that either, and read_some() on a connection a client left open is unbounded. Verified: all nine previously-hanging tests pass on ubuntu:24.04 (Release, matching the CI configuration) and the full suite still passes on macOS.
GCC 11 and 12 emit the operator() of a coroutine lambda declared inside a
function with C language linkage twice, and the assembler rejects the
duplicate symbol ("symbol ... is already defined"). GCC 13 emits it once and
Clang never had the problem, which is why CI stayed green while this file
could not be compiled at all by GCC 11 (Ubuntu 22.04, RHEL 9) or GCC 12
(Debian 12).
Isolated to a minimal repro: a coroutine lambda inside `extern "C"` fails on
both compilers, the same lambda outside it compiles, and a non-coroutine
lambda inside it compiles. Three coroutines here sat inside the block -- the
tool callback body, the checkpoint summarizer and the approver -- and each
accounted for exactly one of the three reported duplicate symbols. The fourth,
the streaming consumer, sits above the block and always built, which is what
made the correlation exact rather than approximate.
Language linkage is a property of the enclosing function, not of the C++ types
its body happens to mention, and none of this is part of the C ABI -- so
moving the three bodies into the existing anonymous namespace changes nothing
except which compiler can parse them.
The failure lands at the assembly step, after the rest of the build has
already succeeded, so it reads like a build-system fault rather than a source
one.
Verified: full suite passes on macOS (158/158), and this file now compiles
under both GCC 11.4 and GCC 12.3 on ubuntu:22.04.
binding.gyp pins -std=c++17 on every platform branch against the core's C++20, which reads like drift and is not. This target compiles one file, and that file includes <napi.h> and the C header ai_sdk.h -- it never mentions ai::Task or any other C++20-only core type, because it links libai_sdk and crosses the C ABI. 17 costs nothing there and is what lets `node-gyp rebuild` succeed on a user's machine whose toolchain has only partial C++20 support (VS2019, older Xcode). Written down so the next reader does not have to re-derive it, or "fix" it.
GCC 11 and 12 cannot build a working library from this source. Past the assembler error that bindings/c/ai_sdk.cpp was rearranged to avoid, they miscompile the coroutines badly enough that 30 of the 158 unit tests crash -- streaming, sessions, tool execution, the C binding's permission gates. Measured on ubuntu:24.04, where only the compiler varies: GCC 12 fails the same 30 at -O0 and at -O3, against the same libstdc++ 14.2 and the same Boost 1.83 that GCC 13 passes all 158 with. GCC 11.4 and 12.3 fail the same 30 on ubuntu:22.04. So this is GCC's coroutine codegen, fixed in 13, and neither the OS, the standard library nor the optimization level is a factor. The honest outcome on those compilers is therefore a library that builds cleanly and then segfaults. Before, the assembler rejected the file and the user learned immediately; now the build succeeds, so the failure would surface in their program instead of their build log. Refusing by name is the better trade, and these versions are the defaults on Ubuntu 22.04, RHEL 9 and Debian 12 -- not a rare configuration to arrive in. Clang is available on all three and builds this fine. AI_SDK_ALLOW_UNSUPPORTED_COMPILER=ON overrides, for reproducing the breakage. Verified: the gate refuses GCC 11.4 with the intended message, and the override carries it past the gate to fail later on something unrelated. CI is green on GCC 13, Clang and AppleClang.
A compiler requirement is the kind of thing a user needs before they build, not after, so it belongs with the other known limitations rather than buried in the build files.
catch_discover_tests() registers every TEST_CASE as a separate ctest entry, so `ctest -j` runs them as separate processes -- but each file named its temp directory from a counter in a function-local static, and a static is per-process. Every process therefore handed out the same first few names: two tests running side by side picked the same directory, and whichever finished first deleted it while the other was still writing into it. That is what made "MarkdownMemoryStore round-trips records with fidelity" fail under `ctest -j` and pass when run on its own. Four files named their directories this way; the helper now lives in temp_dir.hpp and carries the pid, so a name is unique across processes as well as within one. Verified: 162/162 over three consecutive parallel runs.
describeToolSet() in Node, backed by ai_tool_set_describe_json() in the C
API, reports the tools as {name, description, inputSchema}.
The C function follows ai_batch_result_t's ownership rule -- a result
struct whose strings belong to a `_storage` void* released by a matching
free -- rather than returning a bare char*. `ai_tool_set_t` carries no
context, so there is no ai_last_error() to report a failure through, and
every other tool-set call returns ai_status_t.
Entries are ordered by name. A ToolSet is an unordered_map underneath, so
without the sort a set would describe itself differently from one process
to the next -- and the sets this is for (an MCP server's tools, a
permission-wrapped set) are exactly the ones you cannot otherwise see into,
and the ones worth comparing.
The Node wrapper maps the C API's input_schema to inputSchema, so the
public types stay camelCase like the rest of the package.
Also corrects the CHANGELOG's test counts, which read 117 and 50 against an
actual 162 and 60.
Unwrapping a plain {} yields a null handle. Guarding that with a
ThrowAsJavaScriptException made it worse, not better: Unwrap has already
raised a JS exception by the time it returns null, so the second throw
lands on top of a pending one, napi_throw refuses it, and node-addon-api
escalates the refusal to a fatal error that takes the host process down.
Returning without raising is the fix -- the exception the caller needs is
already pending. Verified by a test that passes {} and asserts a throw
rather than a crash; before the change that test aborted the process.
The same unguarded unwrap pattern still exists in the older entry points
(withPermissions, agent creation); those are pre-existing and unchanged.
The note cited the suite as it stood (158) rather than as it ships, and told 22.04 users to reach for Clang without saying that this project builds Clang with -stdlib=libc++, which none of those distributions install by default -- so following it as written fails on every standard header. The PPA route is the one that actually works there, and is now named first.
…ions Passing a non-wrapper to one of eighteen native call sites aborts the process rather than throwing. It is not a regression and the fix is mechanical but wide, so it is recorded rather than quietly left for someone to hit.
Session.sendStream returned its failure status without emitting a terminal event, so the binding's fallback -- which exists to stop a consumer waiting forever -- synthesized a *finish*. The caller saw a turn that succeeded and produced nothing: zero tokens, an empty finishReason, no error. That is the one outcome a consumer cannot detect, because checking for an error is exactly what finds nothing. The sibling entry point, ai_stream_text, has always emitted AI_STREAM_ERROR from its catch. The session entry point did not. Emit it there too, and make the binding's fallback fail closed: a call that ends without saying how it ended is a failure, so it now reports one rather than a finish. Which failures this covers is narrower than a status code: a 401, 429 or 500 arrives as a response and was already surfaced from inside the stream. What reached the dead catch were the failures with no response at all -- refused connection, DNS failure, timeout -- and only for an agent with no tools, since a tool set routes the same failure through the stream instead. The new test drives both shapes against a closed port.
2664390 fixed the session stream's silent success in the C binding, but the only test for it went through the Node binding -- which meant the contract was pinned one layer up from where it is implemented, in a suite that needs a built addon and spawns a process per case. The C API is the layer Python, Rust and Go sit on, and it had no coverage of any streaming error path at all: that gap is why the bug reached a release candidate. The three streaming entry points are now checked directly, against a port the test binds and closes. Verified to discriminate rather than merely pass: with the fix reverted, ai_session_send_stream reports "events emitted: 0" and fails while the other two pass. The first draft segfaulted instead of failing -- the helper read back() off the empty vector before checking it was non-empty, so the failure it exists to report crashed the run. It now requires an element first, which is the whole point: a stream that emitted nothing has to be reported, not tripped over.
ai_tool_result_t.output_json is borrowed, not owned -- the library parses it before the callback returns and keeps no reference afterwards -- but the header said nothing about it, and both bindings read it the other way. Python and Node each allocated a fresh buffer per call with `new char[]`, and nothing on either side of the call freed it, so every tool call leaked its output for the life of the process. Measured rather than reasoned: 150 tool calls of 1 MiB grew RSS by 155 MiB. Proportional to what the tools return, in a loop this SDK exists to run for days, and invisible to every test that checks values rather than memory. The buffer is a thread_local string in each binding now -- reused by the next call on that thread, and released when the thread exits. thread_local rather than a local because the copy happens after the callback returns, and rather than one shared buffer because tool calls run concurrently. The regression test drives 150 tool calls in its own process and fails on RSS growth. Run against the old code it reports the full 155 MiB, so it is measuring the leak and not the machine.
Every one of these was found by building the prebuild in a clean ubuntu:22.04 container rather than by reading the workflow, and two of them would have shipped. The static C++ runtime was never applied. The guard tested CXX_COMPILER_ID instead of CMAKE_CXX_COMPILER_ID -- not a CMake variable at all, so the comparison was false on every compiler and the flags were skipped while the build reported the option as on. libai_sdk.so therefore depended on the build machine's libstdc++: GCC 13 wants GLIBCXX_3.4.32 and Ubuntu 22.04 has 3.4.30, so it would have failed at require() on exactly the systems the 22.04 matrix entries exist to serve. The only symptom was a warning reading "the compiler is GNU, not GNU". A prebuild recorded an empty rpath entry. binding.gyp supplied the loader-relative rpath twice, once per-OS and once through a shared variable; the shared one reached the Makefile unquoted, so the shell read $O as an unset variable and expanded it to nothing. An empty rpath element is not ignored -- it means the current working directory. The duplicate is gone and the prebuild now records exactly one entry, asserted by value after the link. The publish gate could not notice a missing platform. check-prebuilds.cjs checked the directories that happened to be present, which answers "is what arrived complete?" -- and answers yes for a tarball carrying one platform. install.cjs exits 1 when no prebuild matches and has no source fallback, so that tarball is a failed `npm install` for everyone who is not the publisher. It now requires a declared set. Windows is not in it: there is no recipe, and saying so up front beats discovering it at publish time. CI installed a Boost the build cannot use. Ubuntu 22.04's repositories stop at 1.74 and the build requires 1.82, so the Linux leg could not configure at all. It now unpacks Boost 1.83 headers to /opt/boost -- headers only, because the project links Boost::headers and never compiles it. BOOST_ROOT as an environment variable was load-bearing and had only been established by reading FindBoost.cmake; it is measured working in the container now. GCC 13 comes from the toolchain PPA because the image's g++-11 is refused by name -- GCC 11 and 12 miscompile this project's coroutines into a library that segfaults. Two checks could not fail, in opposite directions: - The relocatability whitelist rejected every library on Ubuntu 22.04, which spells system paths /lib/<triple>/..., and it rejected a correctly-rpath'd libai_sdk.so because ldd prints the path it *resolved* rather than the one it was given. It passed on macOS and failed on Linux for an artifact that was fine on both. - The floor report grepped for GLIBCXX_ under `set -e` with pipefail. Once the runtime was linked in statically there were no GLIBCXX_ symbols to find, so grep exited 1 and killed the script -- at the exact moment it was about to report the artifact as portable.
The published artifact had no readme and no licence text. `license: "MIT"` was set in package.json, but npm only auto-includes the licence from the package root, and the MIT text lives at the repository root -- so the tarball carried the label without the notice it refers to. MIT requires the notice to accompany the distribution, so that was a compliance gap as well as a bad first impression. The manifest was written as a working directory rather than a published package: no repository, homepage, bugs or keywords either, all of which are now set. The README is verified rather than illustrative. Every call in it was run against the binding through the mock provider, including the callable provider form, the streaming event loop and the session entry points, and the documented event-type list was checked against what the stream actually emits. A README showing an API the binding does not have fails on the first thing a user tries.
Two independent faults, neither observable from a developer machine. darwin-x64 was pinned to macos-13, which GitHub retired on 2025-12-04. A job on a retired label does not fail to build -- it dies before starting, with "The configuration 'macos-13-us-default' is not supported", so the step never runs and there is nothing in the log that reads like a build error. Moved to macos-15-intel, the last Intel image GitHub publishes. The macOS dependency step exported OpenSSL_DIR. The project resolves OpenSSL through find_package(OpenSSL REQUIRED), i.e. FindOpenSSL.cmake in module mode, which reads OPENSSL_ROOT_DIR; OpenSSL_DIR is the hint for config mode and FindOpenSSL never consults it. Measured rather than reasoned, with Homebrew hidden to reproduce CI: OpenSSL_DIR alone -> "Could NOT find OpenSSL"; OPENSSL_ROOT_DIR alone -> found; neither -> not found, so the discrimination is real. On a developer's Mac Homebrew is already on the default search path and configure succeeds whatever the variable says, which is why this could not be caught locally. Either fault alone is fatal, and not in the way it looks: `assemble` sits behind `needs: prebuild` with no `if:`, so one dead leg skips it and `npm pack` never runs. The symptom is not a broken package, it is no package, and the tarball artifact simply does not appear. Also recorded, not changed: ubuntu-22.04 entered deprecation on 2026-09-17 and retires 2027-04-17. The Linux legs need its glibc 2.35 floor, so the migration is a 22.04 container on a newer runner, not a move to ubuntu-24.04 -- which would raise the floor to 2.39 and drop every Ubuntu 22.04 and Debian 12 user to an install that fails outright, since install.cjs has no source fallback.
The transport classifies retryable failures -- 408/429/5xx set is_retryable(), and a Retry-After reaches retry_after() -- but nothing consumes either. retry.hpp holds a complete retry_async with backoff, jitter and a get_retry_delay that already prefers the server's Retry-After over its own backoff, and no file in src/, providers/ or bindings/ includes it. max_retries is copied between option structs, including in tool_loop_agent, and never read. Verified by grep rather than reading: zero include sites for retry.hpp and zero call sites for retry_async across the source tree. Worth stating rather than leaving implicit, because the failure mode of a retry option that does nothing is a caller who believes they have protection they do not have.
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
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.
Release prep for 1.0.0. The through-line is that everything here was verified against a running system rather than reasoned about: 158 C++ tests and 58 Node tests, plus a packed tarball installed into an empty project outside the repo.
Permission denials carry a reason (core, C, Python, Node)
A policy or approver could refuse a call but not say why, so a model
was told only that the call failed and retried variations of it --
each retry another prompt for a user who had already answered.
npm distribution
Testing