fix(delivery): avoid E2BIG on large settings.local.json (#95)#97
Merged
Conversation
The previous in-memory chain in apply_settings() embedded the full settings JSON blob into single sqlite3 argv elements — six times in strip_agmsg_event, four times in add_event_entry, four times in prune_empty_hooks. On Linux the per-argument cap MAX_ARG_STRLEN is 131072 bytes, so settings files above ~21 KB caused 'set' to die with E2BIG before sqlite3 even started. Rework the chain so each stage reads from a temp file via sqlite3's readfile() and writes back to that file, then atomically mv the final state over the user's settings.local.json. The settings blob never appears in argv any more, so the only ceiling left is sqlite3's own limits (well past anything realistic). - strip_agmsg_event -> strip_agmsg_event_file - add_event_entry -> add_event_entry_file - prune_empty_hooks -> prune_empty_hooks_file - read_settings_escaped removed (no callers left) - delivery.sh status also switched to readfile() for the count probes Tests: add three regression cases for set monitor / set both / set off operating on a ~25 KB settings.local.json. 25 KB * 6 = 150 KB which is > Linux's MAX_ARG_STRLEN; pre-fix the tests fail with E2BIG on Linux (macOS has ~1 MB per-arg headroom so it survives 25 KB even pre-fix, but the new tests still pass post-fix everywhere). All three confirm the user's permissions.allow array is preserved across the rewrite. Closes #95.
agmsg's value prop includes 'no python required'; the test should follow the same constraint. Build the inflated permissions.allow JSON via sqlite3 json_set instead of a python3 heredoc so the test runs in minimal containers (Debian slim, distroless-style) where python3 isn't preinstalled.
Owner
Author
Linux verification (record)Verified on a Debian 12 container after the python3-free test rewrite ( Environment
Post-fix runPre-fix simulationReverted All three regression tests fail pre-fix, confirming they would have caught the original bug. Raw E2BIG reproduction (sanity)Direct invocation of Line 64 (pre-fix) is the Sanity restoreBack to 3/3 green. Safe to merge. |
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.
Summary
Fixes #95. Reworks
apply_settings()so the settings JSON is read via sqlite3'sreadfile()from a temp file at each stage instead of being interpolated into thesqlite3argv. The old in-memory chain embedded the blob 6× instrip_agmsg_event(and 4× in the add/prune helpers), so settings files above ~21 KB tripped Linux'sMAX_ARG_STRLEN(131072 bytes per single argv element) anddelivery.sh setdied withE2BIG.Affected agent types: claude-code and codex (both use the generic
apply_settings()path).gemini/antigravity(markdown) andcopilot(template rewrite viarm -f+ heredoc) are not affected.Diff shape
strip_agmsg_event→strip_agmsg_event_file(reads & writes a path; SQL referencesreadfile('$path'))add_event_entry→add_event_entry_fileprune_empty_hooks→prune_empty_hooks_fileread_settings_escapedremoved — no callers left.apply_settings: in-memory$settings_escchain → temp-file chain. The user'ssettings.local.jsononly changes at the end of the chain viamv, so a mid-chain failure no longer leaves a partially-modified file.delivery.sh status: the three "count entries" probes also switched toreadfile()(they had a much higher threshold — ~130 KB — but it cost nothing to fix while in the file).No behavior change for normal-sized settings. The functions take paths instead of strings, but they only had internal callers.
Tests
Three new regression cases in
tests/test_delivery.bats:delivery set monitoron a ~25 KBsettings.local.json(matches the reporter's ~32 KB case scaled down to stay portable). 25 KB × 6 ≈ 150 KB > Linux's per-arg cap, so the test fails on Linux pre-fix.delivery set both— exercises the longest chain (3× strip + 3× add + 1× prune).delivery set off— exercises strip-only, with an existing agmsg-owned entry plus 600 user permissions, confirming idempotent strip on a large file.All three preserve and verify the user's
permissions.allowarray length (600 entries) across the rewrite.Full suite: 71/71 pass locally (68 existing + 3 new).
Platform notes
MAX_ARG_STRLENis fixed at 32 pages = 131072 bytes per single argv element, regardless ofARG_MAX. This is where the reporter hit the wall.kern.argmax≈ 1 MB; a 32 KB settings file (6× = 192 KB) survives there, so the bug never surfaced for mac-only developers. The new tests still pass on macOS post-fix.Test plan
delivery.sh set monitoragainst a project with a settings file ≥ 25 KB on Linux; verify no E2BIG and the existing permissions block is intact.delivery.sh statusagainst the same file shows correct hook entry counts.Closes #95.