Repository navigation
Integration: consolidated bugfix wave (development -> main) - #131
adriannoes wants to merge 68 commits into
Conversation
Replies for a disconnected webchat conn_id stayed on the shared outgoing queue. After 64 of those, every live client failed to send until restart. Discard frames on unregister, and do not enqueue when no socket is registered. Supersedes #115.
file_read returned success after clipping to the caller buffer. The agent passes a 4096-byte slot, so a later write_file could replace the file with that prefix and delete the rest. Supersedes #114.
The kept tail starts at max_ctx-1, but the summary window stopped one message earlier, so that turn was deleted from the session and never summarized. Supersedes #102.
Compaction deleted every message before the kept tail even when the summary source filled up first. A later short line replaced the truncated long line, so that history was removed without being summarized. Supersedes #108.
The vendored SQLite build is SQLITE_THREADSAFE=0, and the gateway HTTP thread shares that connection with the main loop. Dashboard memory, session list, and cron routes, plus cron_poll's due-job query, touched g_db without agent_lock. A dashboard refresh during a turn or a cron poll is undefined and can crash the process or corrupt memory.db. Supersedes #104.
An interval whose seconds do not fit in now+sec wrapped next_run into the past. Delivery ack recomputed the same sum, so the job stayed due and the agent ran it on every poll. Supersedes #105.
strtol and strtoll accepted a numeric prefix, so at:2026-10-08 stored next_run 2026. That one-shot was already due, ran on the next poll, and ack deleted it. Comma lists such as 1,3,5 matched only the first value and still scheduled. Supersedes #111.
getUpdates used the API default of 100 updates. A backlog larger than the 256 KiB response buffer made curl abort before the offset advanced, so every later poll retried the same batch and the inbox stalled. Messages that omit from.id returned a hard error and pinned the same way. Request one update per poll, and skip a message with no sender id so the following update can be fetched. Supersedes #112.
Telegram treated a finished sendMessage as success, so an HTTP 400 for a reply over 4096 characters still acked cron and the user never saw the text. Discord refused the whole reply above 2000 characters, so the send failed and a due cron job re-ran the agent on every poll. Supersedes #117.
Dashboard and SIGHUP reloads updated OpenAI and the local provider but left Anthropic on the config captured at init. The default provider then kept sending the previous model and max_tokens. Supersedes #106.
Claude often returns several tool_use blocks in one assistant turn. Each result was posted as its own user message, which the Messages API rejects, so the model never saw the tool output after the tools had already run. Supersedes #107.
Init marks llama-server unreachable once. The default chain puts local last, so router recovery never clears that flag and offline chat stays dead until restart. The next chat probes again before refusing. Supersedes #110.
The command child inherited the agent environment, so printenv inside sh -c returned provider API keys and bot tokens on the tool stdout channel. Replace that environment with PATH, locale, and workspace HOME before execl. Supersedes #109.
The shell and sandbox wait loops only advanced elapsed time when poll returned idle. A command that wrote faster than the poll slice never hit shell_timeout_sec or sandbox_exec's timeout, so the agent lock stayed held until the output buffer filled. Count monotonic wall time in both loops and still report the deadline when the child exits because the pipe was closed. Supersedes #116.
…mespaces GitHub-hosted runners reject unshare(CLONE_NEWUSER), so the sandbox_exec success-path tests skip there (namespace isolation failed) and the wall-clock rewrite of drain_pipe plus the secret-free child environment stayed uncovered on CI, dropping the agent-core coverage gate to 79.8%. Expose the pipe helpers under SHELLCLAW_SANDBOX_TEST (CR-21 pattern) and link them from tests/test_sandbox.c, which now covers poll_remain_ms bounds, the monotonic clamp, drain-until-EOF and the wall deadline with a trickle writer, plus the scrubbed child environment via a forked probe.
…ge capture scripts/coverage.sh captures --directory src only; a unit-test object in build/ produced a .gcda the capture never saw, so drain_pipe stayed uncovered and the agent-core gate fell to 79.1%. The ws_test.o pattern already keeps its test object under src/ for the same reason.
The unit-test step builds src/sandbox/sandbox_test.o without --coverage in the same runner workdir; make clean did not delete it, so the coverage build reused the uninstrumented object and the capture lost the drain_pipe traces again. Precedent: WS_TEST_O is pruned the same way.
Add the Unreleased Fixed bullet for the file read bounds fix so the behavior change is changelogged like the other refuse-vs-truncate fixes in this tree. Review of #120 flagged the missing entry; docs only, no behavior change.
The oversize regression used a 32-byte result buffer, smaller than the ~49-byte error JSON, so snprintf truncated the message and the assertions only proved a bare "error" substring. Give the oversize case a 56-byte buffer (still below the 64-byte fixture) so the exact refuse-to-truncate error and the "bytes" field can be asserted, and cover the last boundary: a read of exactly max_len bytes fails where max_len - 1 already succeeds. Test-only change per review of #120.
DELETE /api/sessions/{id} ran session_delete() on the lws service
thread with no agent_lock(), leaving the exact dashboard-vs-agent
SQLite race this branch sets out to close (SQLITE_THREADSAFE=0, one
shared g_db). Every sibling dashboard g_db handler already holds the
lock, and handle_message /reset locks this same call.
Wrap the session_delete() call in agent_lock()/agent_unlock() like
the sibling handlers and build the HTTP response after unlock, so the
route honors the contract documented in agent.h.
The route is now exercised by test_gateway_db_lock: seed a session
with session_save() and DELETE it through dispatch_route. That
requires session_delete to call memory_sqlite_entry_probe(), matching
the other probed entry points; without it the hook cannot observe the
call and the test would stay green while the bug is present. Verified
red first (probe present, lock missing: FAIL g_sqlite_unlocked),
then green after the lock.
The parse-failure branch of cron_ack_delivery moves a recurring job to now+365d without a trace. Since the stricter schedule parser (interval overflow, partial-number rejection), a stored job created with a now invalid schedule (e.g. a legacy comma list) would silently stop firing for a year after its first delivery. Log the job id and schedule on that branch, house style (shellclaw: cron:), before the row is freed (cron_job_row_free clears row.schedule, so it stays ahead of the branch). Fail-close behavior is unchanged.
Document the three consolidated fixes under Unreleased/Fixed: dashboard
and cron SQLite serialization including DELETE /api/sessions/{id},
interval-overflow rejection, and partial-number schedule rejection.
Also note the stored-jobs behavior change: a stored schedule the stricter
parser rejects now snoozes +365d on ack and is logged, so the gap is
diagnosable instead of silent. Reviews of the sibling consolidations
called out the same changelog gap.
tg_send and discord_send carried a copy-pasted piece[] loop (bounded copy, rune-safe cut, POST, advance). Move it once into channel_common.c as channel_send_chunked(text, max_bytes, post, ctx, fail) with the NUL-terminated piece buffer, so bounds-checking is a single concern and the piece copy lives in one place. Both channels now post through a small callback; this also shrinks discord.c, which is past the 1000-line guideline. channel_chunk_fail_t reports the failing chunk index and byte offset so a caller can log partial delivery, and the header documents two_limits contract notes: the chunk_len fallback when the first rune is wider than max_bytes, and that limits are BYTES even where Telegram counts characters (conservative so nobody 'fixes' it to code points later). Cover the helper network-free through the post callback: concatenated pieces reproduce the input exactly (multibyte, 4-byte cap), single-piece fast path, a callback failing on chunk 2 reports index 2 / offset 8, and invalid-argument and empty-text contracts. Extend the chunk_len tests with a 4-byte code point at the split edge. Refs: #123
When a chunk POST fails mid-reply the send returns non-zero and dispatch skips the cron ack, so the job re-runs and any already -delivered chunks are delivered again (at-least-once). Log the failing chunk index and its byte offset in both channels so the duplicate boundary in a re-run is traceable from the daemon log. Refs: #123
Telegram allows about one message per second per chat; a 32 KiB cron or chat reply is up to 8 back-to-back sendMessage calls at the 4096 cap, and a 429 aborts the rest of the reply (which then re-runs, see partial-delivery logging). Pause 500ms between consecutive chunk POSTs — never before the first — to stay under the chat rate. Full 429 handling (parameters.retry_after parse + wait + retry) is a deferred follow-up; this is the deliberate lighter alternative. Refs: #123
WebChat stalling every live client until restart after ~64 abandoned replies is an operator-visible availability fix, so give it a Fixed bullet under Unreleased alongside the other gateway/WebChat entries.
ws_send_to intentionally frees the frame and returns 0 when the target socket is absent, unregistered, or closed (the no-live-wsi path), but the header only promised '0 on success'. State the drop in the contract so webchat_send callers can rely on the actual behavior.
The three fixes carried by PR #123 are user-visible delivery fixes: getUpdates batch resilience (a bad batch no longer pins the offset and kills polling), sendMessage delivery actually verified (HTTP 400 is not success), and long replies chunked for Telegram/Discord within platform limits. Record them under [Unreleased] Fixed, matching the existing bullet style. Refs: #123
Compaction wrote the summarized transcript before the reply, so a failed provider call replaced the stored session with a summary the user never accepted. When the kept tail already starts with a user turn, fold the summary into that turn so Anthropic does not reject the session for consecutive user roles. Supersedes #126.
monotonic_elapsed_ms was duplicated verbatim between sandbox.c and shell.c (PR #125 review, Should Fix 4). Export it and poll_remain_ms through sandbox.h so both timeout loops run one implementation, and drop the two pieces of dead weight the review called out: - the have_clock fallback: CLOCK_MONOTONIC failure is not a realistic case (vDSO), and if it ever failed the deadline simply never fires while poll slices stay bounded; - the remain < 1 clamp in poll_remain_ms: unreachable, both loops break at elapsed >= timeout before calling the helper. The sandbox_exec doc now states that the timeout clock starts only after the isolation handshake (review Nice item 4). test_sandbox.c takes the two helper prototypes from the header (the drain_pipe prototype stays: it only becomes a global under SHELLCLAW_SANDBOX_TEST) and drops the assertion that covered the removed clamp branch. No behavior change: poll bounds, monotonic clamp, drain EOF, wall deadline and trickle timeout tests stay green. Refs: #125
# Conflicts: # CHANGELOG.md
fix(gateway): drop WebChat frames for closed sockets
lws_callback_on_writable updates the poll set without the per-thread lock. Replies and provider-status broadcasts called it from the agent thread while lws_service ran on the gateway thread, racing pollfd updates and a closing wsi. Queue the frame, cancel the service wait, and mark writable only from EVENT_WAIT_CANCELLED. Supersedes #127.
# Conflicts: # CHANGELOG.md
fix(agent): summary and compaction history correctness
# Conflicts: # CHANGELOG.md
fix(cron): schedule validation and serialized SQLite access
DELETE /api/sessions ran on the HTTP thread while agent_run still held the loaded transcript. session_save then recreated the row, so a dashboard delete during a reply restored the conversation. dispatch_delete_session takes the same mutex /reset already uses. Supersedes #103.
skill_get_content returned success after filling its buffer, so the dashboard editor saved that prefix and deleted the rest of the file. Reads that do not fit now fail, and GET /api/skills/:name answers 413 instead of a truncated 200. Supersedes #113.
# Conflicts: # CHANGELOG.md
fix(providers): Anthropic reload and parallel results, local probe retry
config.example.toml sets agent.model to anthropic/claude-sonnet-4-20250514. Both cloud backends forwarded that string. Anthropic rejects the vendor prefix with HTTP 4xx, which stops the fallback chain before local. OpenAI receives the same Claude id after an Anthropic 5xx and returns HTTP 400, which also stops the chain. Strip a matching vendor prefix and skip a foreign cloud id with a non-HTTP error so the router can continue. Supersedes #118.
# Conflicts: # CHANGELOG.md
fix(channels): Telegram polling resilience and long-reply delivery
# Conflicts: # CHANGELOG.md # Makefile
fix(sandbox,shell): hide host secrets and enforce the command timeout
# Conflicts: # CHANGELOG.md
fix(gateway): wake WebChat writes on the lws service thread
…dation fix(gateway): session delete lock and partial skill body refusal
# Conflicts: # CHANGELOG.md
fix(providers): map gateway model ids before cloud fallback
Merging main moved the 1.0.0 header above the shared Fixed list, which would file the unreleased fixes under the tagged release. New bullets stay under [Unreleased]; the 1.0.0 list keeps its own section.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: consolidated bugfix wave (development -> main)
Repo context. ShellClaw is a C99 AI agent for Jetson / Raspberry Pi: agent loop and SQLite sessions in src/core, providers (Anthropic, OpenAI, local), tools (shell, file, cron), channels (Telegram, Discord, WebChat), an lws gateway, and a Linux namespace / Landlock / cgroup sandbox. One test binary per module. agent_lock() is a non-recursive mutex that guards the shared SQLite handle (SQLITE_THREADSAFE=0). Tool config uses the tool_X_set_config setter-global convention listed as known debt in AGENTS.md.
Summary. This PR merges 10 already-reviewed fix PRs (#119 to #130 range) into main: session compaction fixes, agent_lock around dashboard/cron SQLite, sandbox env scrub plus wall-clock shell timeout, Telegram/Discord delivery verification and UTF-8 chunking, Anthropic live reload and grouped tool_result, WebChat queue/wake fixes, read_file refusal, and per-backend model id mapping. I read the full diff in five slices (core, channels, gateway, providers, sandbox/tools/Makefile). I found no memory-safety bug, deadlock or broken contract, and agent.c / ws.c / sandbox.c lock and cleanup paths check out. Built with CC=gcc, the test_agent, test_dispatch, test_reload, test_skill, test_channel, test_ws, test_sandbox, test_shell, test_file and test_cron binaries build and pass. test_telegram, the provider tests and the GATEWAY=1 tests could not be run in this environment (libcurl / libwebsockets dev packages missing).
Must Fix
None.
Should Fix
-
Custom OpenAI endpoints lose vendor-prefixed ids (
src/providers/openai.c:44,src/providers/provider_common.c:105-123).provider_model_id_for_backend(configured, "openai")runs regardless of[providers.openai] api_base. An OpenAI-compatible gateway such as OpenRouter needsopenai/gpt-4ooranthropic/claude-...verbatim. With this change the first is stripped and the second makes OpenAI return -1 ("skipped").config.example.tomlships a[providers.openrouter]block and anapi_basehint, so this is a plausible setup. Apply the mapping only when the endpoint is the defaultapi.openai.com, or add an opt-out.tests/test_openai.chas no custom-endpoint case. (Carried over from the #130 review.) -
Other vendor prefixes still reach the wrong backend and stop the fallback chain (
provider_common.c:75-82,:120).vendor_tag_is_cloudonly knowsanthropicandopenai.meta-llama/llama-3oropenrouter/anthropic/claude-3go through unchanged to Anthropic and OpenAI, get a 4xx, andprovider_error_allows_fallback_retrystops, so local is never tried. That is the failure the CHANGELOG says is fixed. For the Anthropic backend, skip any prefixed id whose vendor is notanthropic. Also handleanthropic/and/foo(empty id after the strip):provider_common.c:116returns the raw string, which Anthropic rejects with a 400. Matching is case-sensitive while the router matches provider names case-insensitively (Anthropic/claude-xpasses through).tests/test_provider.chas no multi-slash, empty, other-vendor or case cases. -
Compaction can cut a UTF-8 character in half (
src/core/agent.c:135-138).summary_source_append_truncateduses%.*swith a bytecut, so the stub can end mid-codepoint and the invalid UTF-8 goes into the JSON body of the summarizer call. If a provider rejects it,provider->chatfails, compaction returns -1, nothing is persisted (correct since this PR) and the same stub is rebuilt every turn, so compaction stays broken for that session. Backcutup while(c[cut] & 0xC0) == 0x80. No test truncates non-ASCII text.channel_outbound_chunk_leninchannel_common.calready does this and could be reused. -
Partial compaction pass can shrink nothing (
agent.c:252-262,:290-291,:317-321). If a line does not fit andused > 0,next_unsummarized = imakesstartsmall. When the tail then starts with an assistant turn (merge == 0), one message is replaced by one summary and the count is unchanged. With two messages of about 32 KB or more, each turn adds 2 and removes at most 1 until the 128 KiBSESSION_JSON_MAXcap, after which compaction pays an LLM call per turn and cannot save. The summary also ends up at the head of an array longer thanmax_ctx, andparse_session_into_messages(agent.c:437) takes the lastmax_ctx, so the model may never see it. Related: the stub path writes into whatever room is left, which can be a few bytes. Whenused > 0, prefernext_unsummarized = i; break;so the big message gets the full buffer next pass. The oversized-message tests assert the count shrinks, not that the summary stays in the sent window. -
Reply to a failed chunk is lost on interactive channels, and the comments claim otherwise (
src/channels/telegram.c:433-435,src/channels/discord.c:966-968). The comments say at-least-once delivery because the cron ack is skipped. That holds only forcron(dispatch.c:19-20acks only forch->name == "cron"). On Telegram,last_update_idadvances before the reply is sent, so a failure on chunk 2 of 3 leaves the user with a partial reply and no retry. For cron-to-Telegram, a retry re-sends chunks 0..k-1 as duplicates. Fix the comments, and consider a short "reply truncated" notice. Telegram 429 is also a hard failure (telegram.c:332-339) while Discord retries; retry once withretry_after. -
WebChat wake path has two shutdown holes (
src/gateway/http.c:143-151,src/gateway/ws.c:93-102). Onpthread_createfailure the context is destroyed butws_set_context(NULL)is never called, leavingg_lws_ctxdangling. Movews_set_contextto after a successfulpthread_create. Separately,ws_wake_servicecopies the context, unlocks, then callslws_cancel_service(ctx);http_stopcan destroy the context in that gap. Call it underg_mutex(it only writes a pipe). I could not construct a reachable crash from the current callers, so this is hardening. -
Anthropic live config is read twice without synchronisation (
src/providers/anthropic.c:410-423,:528-531). Model andmax_tokenscome from two separate reads ofs_anth_cfg, so a reload between them mixes old and new. Take one snapshot per request. Alsoanthropic_live_config_for_test(anthropic.c:534-537,provider.h:143) is outside#ifdef SHELLCLAW_TEST, which leaks a test hook into production builds. No test coversprovider_anthropic_set_live_configreaching a chat call. -
Re-probe cost on the agent thread (
src/providers/local.c:200-207). The throttle bounds frequency (15 s) but each probe is synchronous curl underagent_lockand can block about 6 s (/v1/modelsthen/health, 3 s each). UseCLOCK_MONOTONICinstead oftime(NULL), and either shorten the timeout for the throttled probe or state the cost indocs/LOCAL_INFERENCE.md. -
Dashboard routes now stall the lws service thread behind a running LLM turn (
src/gateway/routes.c:511, 538, 581, 642, 665, 679). All/apiDB routes block inagent_lock()for the wholeagent_run, and/health, WS frames and the wake are served by the same thread. It matches/asapand is not a deadlock, but it deserves a CHANGELOG note, or atrylockreturning 503. -
Test gaps worth closing before release.
tg_sendpacing / chunk wiring andtg_polllimit=1with offset advance on a skipped update have no HTTP-level test (tests/test_telegram.c).tests/test_sandbox.c:379acceptsstrlen(out) == 0so it can pass with no timeout message. The unit scrub test coversLD_PRELOAD,PATH,HOMEand the secret, but not thatLANG/TERM/TZsurvive or the fallbackPATH. Cron tests lackinterval: 60,interval:+60,interval:0, negatives, and the snooze log line.test_wsstub tests are single-threaded and do not exercise the cross-thread race.
Nice to Have
shell_timeout_secis cast tointwithout a clamp (src/core/config.c:728) and multiplied by 1000 inint(src/tools/shell.c:240). Values above about 2.1M seconds overflow, and a negativepolltimeout blocks forever. Operator-only, but easy to clamp.- Sandbox timeout can reach about 2x:
read_isolation_bytewaits the full timeout (sandbox.c:514) anddrain_pipethen starts another (:530). The header says so; the CHANGELOG wording is looser.monotonic_elapsed_ms == -1makes the deadline fail open (shell.c:144); atime()fallback is cheap. sandbox_scrub_command_environment(sandbox_env.c:50-77) callsstrdup/setenv/clearenvafterfork()in a multithreaded process. A static env array built in the parent and passed toexecveremoves the question.- Cron snooze log prints model-supplied
scheduleunescaped (cron.c:436).strtollstill accepts leading whitespace and+(interval: 60,at:+5). agent.c:309-310:if (!dup) continue;silently drops a middle message on OOM and persists the session; return -1.fold_summary_into_userdrops a non-stringcontent; return -1. The folded summary has no label, so the model cannot tell it from user text (Summary of earlier conversation:would do).skill_get_contentreturns -2 for a NUL byte in the file (skill.c:359), which the route reports as "exceeds N byte edit buffer". Wrong message only.telegram.c:225-226leaksrooton OOM in the caption path (not from this PR).limit=1opens a new curl handle per update, so backlog drain is slower.- Style: unindented
RUN(test_unregister_drops_pending_outgoing());intests/test_ws.c,MU_RUN(test_getupdates_url_requests_one_update);intests/test_telegram.c:268,ws.c:409, a missing blank line atanthropic.c:331-332. - Makefile:
tweetnacl.cfails under clang with-Werror=unterminated-string-initialization(the defaultCChere). Not from this PR, but it breaksmake test_*on clang-only hosts. - The CHANGELOG could mention that
read_filenow fails where it used to clip, and that dashboard routes can wait for an in-flight turn.
Positive Highlights
channel_send_chunked/channel_outbound_chunk_len(channel_common.c:34-101) is a small, bounded, shared helper with guaranteed progress and tests across 2-, 3- and 4-byte boundaries, round trip, failure index and offset. Byte-based limits are conservative for both Telegram and Discord.dispatch_delete_sessiontakesagent_lockonce, is called only from a route that does not hold it, andtests/test_gateway_db_lock.cproves the lock is held (trylock from a thread that owns it) for sessions, memory and the four cron handlers. I also checkedroutes.cfor any remaining unlockedg_dbaccess and found none.- Moving
session_saveout ofcompact_session_via_llmfixes the data-loss path cleanly, with a failure test that checks the raw session is unchanged. - The monotonic deadline helpers are shared by the sandboxed and unsandboxed paths, the group is killed before
waitpid, and the trickle tests scan/procfor leftoversleepprocesses. The secret scrub test checksprintenvand/proc/1/environthrough the realsandbox_exec.
Verdict: no blockers. Items 1 and 2 are the ones I would fix before tagging, since they can break fallback or custom-endpoint setups that worked on the previous release.
Sent by Cursor Automation: Code review
This branch has not been deployed
1 participant
Summary
Promotes the consolidated bugfix wave from
developmenttomain.development; the originals were closed as superseded with pointers to their replacements.max_context_messages.agent_lock, including the shareddispatch_delete_session()entry point forDELETE /api/sessions/:id.sh -c) and monotonic wall-clock command timeout on both execution paths.channel_send_chunkedhelper.EVENT_WAIT_CANCELLED).read_filerefuses reads that do not fit the buffer; per-backend model-id mapping unblocks the fallback chain; dashboard refuses a partial skill body.CHANGELOG.md[Unreleased]section,docs/SECURITY.mdenvironment model,docs/LOCAL_INFERENCE.mdre-probe note.Testing
static, test, release(46 test binaries) plus cppcheck on its own branch; the coverage gate held at >= 80%.developmentwas merged ontomainbefore opening (no conflicts; the 1.0.0 changelog date was already applied through the wave).