Skip to content

release: land Phase 5 on main (Jetson on-device pending) - #83

Open
adriannoes wants to merge 72 commits into
mainfrom
development
Open

release: land Phase 5 on main (Jetson on-device pending)#83
adriannoes wants to merge 72 commits into
mainfrom
development

Conversation

@adriannoes

Copy link
Copy Markdown
Collaborator

PR body — developmentmain (Phase 5 onto main)

Title: release: land Phase 5 on main (Jetson on-device pending)

Create PR:

gh pr create --repo asap-protocol/shellclaw \
  --base main \
  --head development \
  --title "release: land Phase 5 on main (Jetson on-device pending)" \
  --body-file docs/issueops/pr-development-to-main-v1.0.0.md

Summary

Land Phase 5 on main: GPIO/I2C tools, CUDA local inference path, signed ASAP manifest, gateway /hardware UI (sensor/camera panels deferred to v1.2).

Known pending (not a merge gate): on-device Jetson Orin Nano Super sign-off — docs/JETSON_SIGNOFF.md. Continue product work on main; run the checklist when hardware is available.

Evidence

Artifact Link
Security self-audit docs/SECURITY.md
Benchmarks (Jetson rows still _run on device_) docs/BENCHMARKS.md
Changelog CHANGELOG.md § [1.0.0] / Unreleased
Jetson operator checklist docs/JETSON_SIGNOFF.md
Release runbook docs/RELEASE_V1.0.md

Jetson sign-off

  • Status: known pending — does not block this PR
  • On-device runner (later): SHELLCLAW_HW_TEST=1 make test_hardware_on_device
  • Manual checklist: JETSON_SIGNOFF.md

Pre-merge verification (x86 / CI)

  • CI=true GATEWAY=1 make clean && CI=true GATEWAY=1 make test
  • make static — zero cppcheck findings (when cppcheck is available)
  • make test-sanitize — AddressSanitizer + UBSan (Linux CI)
  • make release binary < 2 MB (CI); hardware backends target < 600 KB

Post-merge (maintainer — not in this PR)

  1. Continue work on main
  2. Optional later: Jetson sign-off, then tag v1.0.0 per RELEASE_V1.0.md Phase C
  3. Pages manifest + marketplace IssueOps when tagging

Test plan

  • CI green on development at merge SHA
  • No v1.2-deferred features claimed as shipped (sensors, camera E2E, deferred skills)
  • Jetson on-device work tracked as known pending, not as a blocker

CI / closed-PR learnings

Harvested from closed-without-merge PRs (#7, #8, #10, #13, #15#18, #20#34, #36#42, #44, #51) and merged sanitizer hardening in #43 (90d323e).

Pitfalls already present in this tree (verified; no extra commits):

What we ran before this PR

Check Where Result
CI=true make clean && CI=true make test macOS (prior session + sanitizer rebuild) pass
make static (cppcheck 2.21) macOS pass
make test-sanitize (ASan+UBSan, GATEWAY=1) macOS Apple Clang pass
make release size macOS 1 248 008 bytes (< 2 MB)
GATEWAY=1 make coverage macOS Homebrew lcov 2.5 + Apple Clang fail locally — empty .info traces; not a Linux GCC issue
Docker / Ubuntu 24.04 mirror this Mac not available — no docker/colima/lima/podman, no Dockerfile in-repo
libgpiod v2 job this Mac not mirrored (libgpiod absent; stub path only)

Linux ubuntu-24.04 (static, test, release, including coverage and LIBGPIOD=0) is the remaining merge gate.

adriannoes and others added 30 commits May 23, 2026 14:58
Board detection, libgpiod GPIO (SFIO-safe), I2C, camera CLI backends,
40-pin tables, hardware_init, seven LLM tools, --detect-board, CI on
ubuntu-24.04 with libgpiod 2.x, and hardware unit tests.

Agent-core coverage gate excludes src/hardware/*; hardware correctness
is enforced by test_hardware_* on Mac and Linux CI.
feat(hardware): Phase 5 slice 01 hardware foundation
System ethernet.h expects u_char/u_short before libwebsockets on some
platforms; include the shim in gateway channel sources that pull LWS.
Expose active board pin tables, libgpiod line status without reconfiguring
SFIO rows, and hardware_resolve_i2c_bus for shared bus selection.
Parse JetPack 6.x tegrastats lines, nvpmodel power mode, and llama-server
probe; fill JSON for GET /api/hardware/gpu on Jetson boards.
Bearer-authenticated board, GPIO, I2C scan, GPU, and v1.2 deferred stubs
for sensors and camera snapshot; dispatch from routes_hardware.c.
Five-tab layout (Board, GPIO, GPU, Sensors/Camera placeholders), SVG pin
grid, GPU gauges on Jetson; embed assets via embed_ui.sh.
Link routes_hardware, gpio snapshot, tegrastats objects; add unit and
gateway test targets; extend embedded UI asset dependencies.
Snapshot holds one mutex and reuses open chips per pass. Output lines
report direction only without requesting lines as output, so periodic
UI polls cannot drive pins low.
Treat non-zero pclose status as failure for tegrastats, nvpmodel, and
pgrep. Build Jetson GPU JSON in a temp object so root stays unchanged on OOM.
Stop polling i2c-scan every 15s; scan on first Board tab visit or via
Scan I2C button. Restore active tab after live data refresh.
Cover POST /api/hardware/board and GET /api/hardware/camera/snapshot.
feat(phase5): hardware Web UI slice and /api/hardware routes
Vendored TweetNaCl with pinned hashes, RFC 8785 JCS canonicalization,
fail-closed OS RNG for key generation, and base64 helpers. Compiler
probes optional TweetNaCl warning flags for older GCC on CI.
Serve SignedManifest at /.well-known/asap/manifest.json with JCS signing,
atomic key persist/rotate with rollback on failure, Manifest v2.4
hardware/inference profiles, config/skill metadata, and --rotate-keys CLI.
Includes regression tests for manifest, keys, gateway discovery, and ws.
…nstall

Scripts to fetch pinned GGUF models, build llama-server on Jetson/RPi,
install systemd units with per-board env files, and headless install/download
tests.
Document registry compliance, marketplace verification, and registration
prefill. Add GitHub workflow to publish signed manifest artifacts.
feat(phase5): signed ASAP manifest, local inference, and marketplace
Split ASAP manifest build/sign/profiles, cap POST /asap bodies, tighten
camera validation and sandbox GPU blocks, add release docs/skills/tests,
and trim redundant gateway code paths.
feat(release): phase 5 slice 04 — security, docs, and gateway hardening
Operator runbooks for Phase B (B1–B6) and Phase C (PR body, post-tag
manifest/marketplace steps). Issue body ready for gh when issues are enabled.
Cover dashboard summary lines, minimal snapshot location hints, and
Dec 31 year-boundary holiday marking without network I/O.
Assert holidays, weather parse failure, and cold dashboard snapshot in
test_context. Reject invalid bearer tokens on /api/config in
test_gateway_http. Record last_error when all fallback backends fail in
test_router.
Integrate unique coverage from cursor PR #17; dashboard and year-span
cases were already on development via efd9fcf and ea51fe2.
Integrate unique coverage from cursor PR #20: configurable stub-b chat
errors, router chain stop on terminal HTTP 401, and stricter get_context
dashboard strings while keeping development tests from PRs #15#17.
Add handle_message unit tests for /reset, /status, agent success, and
provider failure fallback. Extend get_context tests for holiday flags,
weather API failure, and geo parse failure without regressing prior coverage.
Port PR #25 gateway integration tests onto development: 401 without
token, invalid TOML rollback, and successful persistence to disk.
get_context buffer limits are already covered on development.
Port nager upstream HTTP failure sentinel, holidays-unavailable asserts,
and router unknown-provider plus post-fallback status JSON coverage onto
development without regressing newer get_context and stub-b error hooks.
Port nager upstream HTTP failure sentinel, holidays-unavailable asserts,
and router unknown-provider plus post-fallback status JSON coverage onto
development without regressing newer get_context and stub-b error hooks.
Keep docs, scripts, and defaults aligned after the repo transfer so
manifest, registry, and IssueOps links point at the org.
Enable required code-owner reviews with a named default owner after
the org transfer.
Keep Phase 5 Makefile and config.example.toml. Combine both reload
test suites and retain main's OpenAI tool-call coverage.
Stop treating physical Jetson validation as a merge-to-main gate so
Phase 5 can land and work continues on main.
@adriannoes
adriannoes marked this pull request as draft September 11, 2026 03:35
@adriannoes

Copy link
Copy Markdown
Collaborator Author

CI / closed-PR learnings (pre-merge)

Harvested from closed-without-merge PRs (#7, #8, #10, #13, #18, #21, #28, #34, #37, #38, #44) and the clean landings (#9, #11, #14, #43).

Already in development (no extra source patch from this harvest):

  • test_reload uses GATEWAY=0 + http_reload_stub (avoid undefined reference to http_set_live_config)
  • get_context overflow uses a ≥64-byte buffer and the full get_context payload too large prefix
  • Coverage ≥80% is agent-core only (lcov --remove '*/hardware/*'); unit slice stays GATEWAY=0
  • Vendored -Werror off; TweetNaCl -fwrapv; cJSON float-cast UBSan suppression; GCC format-truncation pragma
  • Dual GPIO jobs: libgpiod v2 present and LIBGPIOD=0
  • Ubuntu libwebsockets 4.3 HTTP/POST/mount path (not Homebrew 5.x)

Still Linux-only residual risk (this is why we wait on Ubuntu CI, not macOS green):

  • GCC -Werror (-Wformat-truncation, -Wstringop-overread) vs Clang
  • libgpiod v2 compile of hardware_libgpiod.c
  • apt lcov + GCC coverage traces (Homebrew lcov on macOS is not the gate)

Jetson on-device sign-off remains known pending, not a merge gate (docs/JETSON_SIGNOFF.md).

Marking this PR draft until Ubuntu 24.04 CI (and local Docker mirror, if it finishes) is green.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Comment thread src/tools/hardware_tools.c
@adriannoes adriannoes self-assigned this Sep 11, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Left a non-blocking comment. Cursor Security Agent reported an unresolved HIGH finding on camera_capture path handling, so this is not approved. Assigned adriannoes for human review; Bugbot was not present on this PR.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Summary

This PR lands Phase 5 on main: libgpiod GPIO, I2C, camera CLI skeleton, Jetson tegrastats + /hardware UI, Ed25519/JCS signed ASAP manifests, llama-server install path, and the v1.0 docs/IssueOps suite. Jetson on-device sign-off is correctly treated as known pending, not a merge gate. The slice work (hardware foundation, Web UI, signing, sanitizer hardening) is real and well documented, but three defects mean the advertised GPIO/I2C/camera agent tools do not actually ship to the LLM, and gpio_write cannot hold a pin even for the two tools that do get registered.

Must Fix

  1. Hardware tools are truncated at 8 slots. src/core/bootstrap.c still has MAX_TOOLS 8. tool_get_all always fills six core tools (shell, web_search, file, cron, context, asap_invoke), then tool_hardware_get_all gets the remaining 2. Default hardware_enabled is 1 (config.c / config.example.toml). The agent therefore exposes gpio_read and gpio_write only. gpio_mode, i2c_read, i2c_write, i2c_scan, and camera_capture never reach dispatch.c. Raising MAX_TOOLS without also widening agent_tool_t flat_tools[8] in src/core/dispatch.c will overflow the stack. tests/test_registry.c stubs every core getter to NULL and uses a 32-slot array, so this is invisible to CI.
  2. gpio_mode_exec use-after-free. mode aliases mode_item->valuestring, then cJSON_Delete(root) frees it, then hardware_gpio_mode(pin, mode, …) and the success snprintf still read mode. tests/test_hardware_tools.c never calls gpio_mode. This becomes live as soon as the tool table cap is fixed.
  3. gpio_write / gpio_mode drop the line request immediately. After a successful write, hardware_libgpiod.c calls gpiod_line_request_release. On gpio-cdev that typically returns the line to the controller default (input / Hi-Z). The tool reports success after a pulse, not a held level. gpio_write is one of the two hardware tools that currently register. Hold per-pin requests until the process exits or gpio_mode sets input.

Should Fix

  • manifest_write_key_file opens with O_CREAT|O_TRUNC and no O_NOFOLLOW. Reads already use O_NOFOLLOW. A planted ed25519.priv.tmp symlink in the keys dir would be followed. Re-check mode bits on the fstat fd, not only the earlier lstat.
  • TweetNaCl randombytes returns on /dev/urandom failure without filling the buffer. crypto_ed25519_keypair probes first, so the public API is mostly fail-closed, but a later callback failure still leaves uninitialized key material. Fail closed (fatal flag / abort) on RNG error.
  • --version in main.c and /status in dispatch.c still print 0.2.0 while SHELLCLAW_RELEASE_VERSION and the ASAP manifest are 1.0.0.
  • CHANGELOG.md claims a 1 req/s camera snapshot rate limit. docs/SECURITY.md says that throttle was removed until v1.2. Align them.
  • .github/workflows/publish-manifest.yml uploads the entire docs/ tree to Pages (SECURITY.md, IssueOps, fixtures). Stage a directory that contains only manifest.json and .nojekyll. Tag publishes also mint a fresh signing key via a temp SHELLCLAW_HOME; marketplace consumers cannot pin a stable publisher pubkey.
  • scripts/download_model.sh treats EXPECTED_SHA256 as optional and ships no default digest. HTTPS URL allowlisting is good; add pinned SHA256 for phi3 and tinyllama.
  • I2C tools cap addr 0x03–0x77 and len 1–256; hardware_i2c_read / write do not. Enforce the same limits in the backend.
  • Camera default_spawn uses unbounded waitpid. A hung gst-launch-1.0 / libcamera-still blocks the process (and any shared gateway worker).
  • Raspberry Pi pin 26 (GPIO7 / SPI0 CE1) is RPI_HDR_GPIO while pin 24 CE0 is SFIO.
  • systemd/llama-server.service: quote $MODEL; add NoNewPrivileges / PrivateTmp / ProtectSystem=strict. Pin publish-manifest Actions to full SHAs.

Nice to Have

  • Jetson on-device sign-off stays deferred as documented. Do not block this PR on physical hardware once the Must Fix items are in.
  • config.c 1261 lines and the tool_X_set_config setter-global convention are already waived to v1.0.1 in AGENTS.md. Fine.
  • src/asap/manifest_build.c (620) and tests/test_manifest_keys.c (809) are over the 500-line house rule; the split from a single manifest file already happened.
  • Validate gpio_write value ∈ {0,1} in C, not only in the JSON schema enum (int_to_line_value treats any non-zero as HIGH).
  • Camera hang/timeout test with a mock spawn; ASan regression for gpio_mode JSON execute; a registry test that uses the real core getters and max_count == 8 (or whatever the new cap is).

Positive Highlights

  • SFIO rejection, pin-range checks, GPIO mutex, and a read-only snapshot path that does not reconfigure outputs for the Web UI.
  • Camera spawn is fixed argv + execvp (no shell), with path character allowlisting, .. rejection, JPEG magic check, and temp unlink on internal errors.
  • Signing path is serious: 0600 keys, atomic write + fsync, O_NOFOLLOW on read, pub/priv consistency, eager key load at gateway start (not on the first unauthenticated manifest GET), JCS goldens vs the Python verifier, and publish-manifest verify_manifest hard-fail (no || echo).
  • /api/hardware/* sits behind the central Bearer gate; 401 coverage exists. v1.2 sensor/camera HTTP capture is honestly stubbed.
  • Release docs (JETSON_SIGNOFF.md, SECURITY.md, HARDWARE_SAFETY.md) match the deferred scope instead of claiming on-device numbers that were never measured.

Linux static, test, release was still running when this review was posted. validate-manifest already passed. Earlier development CI on the CODEOWNERS commit was green. The Must Fix items are logic bugs, not CI flakes.

Open in Web View Automation 

Sent by Cursor Automation: Adrianno’s personal code review

Comment thread src/tools/registry.c
out[n++] = ctx;
if (n < max_count && asap_invoke)
out[n++] = asap_invoke;
n += tool_hardware_get_all(out + n, max_count - n);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must Fix: this append is silently truncated.

tools_init() calls tool_get_all(g_tools, MAX_TOOLS) with MAX_TOOLS still 8 (src/core/bootstrap.c). The six core getters always return non-NULL (shell, web_search, file, cron, context, asap_invoke), so max_count - n is 2. Only gpio_read and gpio_write are copied. gpio_mode, all three I2C tools, and camera_capture never reach the agent, even though hardware_enabled defaults to 1.

Fix both caps together: raise MAX_TOOLS and agent_tool_t flat_tools[8] in src/core/dispatch.c. Widening bootstrap alone overflows flat_tools when tool_count > 8.

Add a test that uses the real core getters (not NULL stubs) and asserts the live tool list contains the hardware names when hardware is enabled.

Comment thread tests/test_registry.c
FILE *f;
config_t *cfg = NULL;
char errbuf[256];
const tool_t *tools[32];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test cannot catch the production cap.

Core getters are stubbed to NULL above, and the buffer is 32 slots. tool_get_all therefore sees n == 0 and copies all seven hardware tools. In the real binary those getters are never NULL, so MAX_TOOLS 8 drops five of them.

Keep the schema checks, but add a path that either links the real tools or asserts tool_get_all(buf, 8) with non-NULL core stubs and expects hardware names still present (after the cap is raised).

Comment thread src/tools/hardware_tools_gpio.c Outdated
Comment on lines +134 to +157
mode = mode_item->valuestring;
if (strcmp(mode, "input") != 0 && strcmp(mode, "output") != 0) {
cJSON_Delete(root);
snprintf(result_buf, max_len, "{\"error\":\"mode must be input or output\"}");
return -1;
}
cJSON_Delete(root);
if (hw_tools_validate_gpio_pin(pin, result_buf, max_len) != 0)
return -1;
if (!hw_tools_gpio_ready()) {
snprintf(result_buf, max_len, "%s", HW_ERR_GPIO);
return -1;
}
#ifdef HAVE_LIBGPIOD
rc = hardware_gpio_mode(pin, mode, errbuf, sizeof(errbuf));
#else
rc = -1;
snprintf(errbuf, sizeof(errbuf), "GPIO not available");
#endif
if (rc != 0) {
hw_tools_json_error(result_buf, max_len, errbuf);
return -1;
}
snprintf(result_buf, max_len, "{\"pin\":%d,\"mode\":\"%s\"}", pin, mode);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must Fix: use-after-free on mode.

mode points at mode_item->valuestring. cJSON_Delete(root) at line 140 frees that string. hardware_gpio_mode(pin, mode, …) and the success snprintf then read freed memory. gpio_read / gpio_write copy ints before delete; this path does not.

Copy into a stack buffer (char mode_buf[8]) before cJSON_Delete. Add an execute-path test in tests/test_hardware_tools.c (ASan will catch this today if you call HW_TOOLS_GPIO_MODE.execute). This is latent until the 8-slot tool cap is raised, then it is on the live gpio_mode path.

Comment thread src/hardware/hardware_libgpiod.c Outdated
}
ret = 0;
done:
release_request(request);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must Fix: the driven level does not persist.

gpiod_line_request_release returns the line to the kernel. On gpio-cdev that typically clears FLAG_IS_OUT and the pin goes back to the controller default (input / Hi-Z). gpioset holds the request for this reason.

gpio_write is one of the two hardware tools that currently register (MAX_TOOLS 8), so this is not a dormant path. The agent reports {"pin":N,"value":1} after a pulse, not a held HIGH.

Keep a per-pin gpiod_line_request * for the process lifetime (mutex you already have). Release on gpio_mode input, hardware shutdown, or process exit. The snapshot path that avoids requesting outputs can stay as-is.

Comment thread src/asap/manifest_keys.c Outdated
g_manifest_keys_test_fail_backup_write = 0;
return -1;
}
fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, MANIFEST_KEY_FILE_MODE);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Fix: writes are missing O_NOFOLLOW.

Reads at line 211 correctly use O_RDONLY | O_NOFOLLOW. This open(O_WRONLY|O_CREAT|O_TRUNC) follows a planted ed25519.priv.tmp / .bak.* symlink and would truncate the target, then write private-key bytes into it.

Add O_NOFOLLOW, fstat the fd, require a regular file, and re-check (st_mode & 0077) == 0 on that fd rather than trusting the earlier lstat. Cover symlink-on-write in tests/test_manifest_keys.c (read-symlink is already tested).

Comment thread src/core/main.c Outdated
@@ -112,6 +115,20 @@ static int parse_args(int argc, char **argv, const char **config_path_out)
printf("%s\n", VERSION);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Fix: --version still prints VERSION (0.2.0 in this file). Manifests and src/core/version.h advertise SHELLCLAW_RELEASE_VERSION 1.0.0. /status in src/core/dispatch.c has the same stale 0.2.0.

Wire CLI and /status to SHELLCLAW_RELEASE_VERSION so a v1.0.0 tag and shellclaw --version cannot disagree.

@adriannoes

Copy link
Copy Markdown
Collaborator Author

Linux CI mirror (local Docker Ubuntu 24.04)

Throwaway ubuntu:24.04 + scripts/ci-local.sh (no in-repo Dockerfile). All GHA-equivalent gates passed:

Gate Result
make static PASS
CI=true GATEWAY=1 make test PASS
LIBGPIOD=0 make test PASS
make test-sanitize PASS
make release size < 2 MB PASS — 1 208 488 bytes
GATEWAY=1 make coverage PASS — core 82.4% (3539/4294)

Caveats vs GitHub Actions: Colima was aarch64 (GHA is x86_64); apt libgpiod-dev in this image was 1.6.3 so both Docker test jobs stayed on the stub GPIO path. Ubuntu 24.04 GHA already ran the real libgpiod-dev job and is green.

Not merging while review is CHANGES_REQUESTED (tool-table cap / gpio_mode UAF / line-request lifetime).

@adriannoes
adriannoes marked this pull request as ready for review September 11, 2026 04:08

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR lands Phase 5 on main: libgpiod GPIO, I2C, camera CLI skeleton, Jetson tegrastats + /hardware UI, Ed25519/JCS signed ASAP manifests, llama-server install path, and the v1.0 docs/IssueOps suite. Jetson on-device sign-off is correctly treated as known pending, not a merge gate. The slice work is real and well documented, but three defects mean the advertised GPIO/I2C/camera agent tools do not actually ship to the LLM, and gpio_write cannot hold a pin even for the two tools that do get registered.

Re-review of e76f020 (no new commits). Linux static, test, release and validate-manifest are green. The author already noted they will not merge until the tool-table cap, gpio_mode UAF, and line-request lifetime are fixed. Those three are still open.

Must Fix

  1. Hardware tools are truncated at 8 slots. src/core/bootstrap.c:24 still has MAX_TOOLS 8. tool_get_all always fills six core tools, then tool_hardware_get_all gets the remaining 2. Default hardware_enabled is 1. The agent therefore exposes gpio_read and gpio_write only. gpio_mode, i2c_*, and camera_capture never reach dispatch.c. Raising MAX_TOOLS without also widening agent_tool_t flat_tools[8] in src/core/dispatch.c:31 will overflow the stack. tests/test_registry.c stubs every core getter to NULL and uses a 32-slot array, so this is invisible to CI.
  2. gpio_mode_exec use-after-free. mode aliases mode_item->valuestring, then cJSON_Delete(root), then hardware_gpio_mode + snprintf still read mode. tests/test_hardware_tools.c never calls gpio_mode. This becomes live as soon as the tool table cap is fixed.
  3. gpio_write / gpio_mode drop the line request immediately. After a successful write, hardware_libgpiod.c:383 calls gpiod_line_request_release. On gpio-cdev that typically returns the line to the controller default (input / Hi-Z). gpio_write is one of the two tools that currently register. Hold per-pin requests until the process exits or gpio_mode sets input.

Should Fix

  • Camera output path has no workspace bound. path_chars_safe() allows / and rejects only .. / shell metacharacters. An LLM-supplied absolute path can overwrite ~/.shellclaw/config.toml or the Ed25519 private key with JPEG bytes. write_file has workspace_only; camera does not. Fix this in the same follow-up as the tool cap, or ignore LLM path in v1.0 and always use the temp file.
  • manifest_write_key_file opens with O_CREAT|O_TRUNC and no O_NOFOLLOW. Reads already use O_NOFOLLOW. A planted ed25519.priv.tmp symlink in the keys dir would be followed.
  • TweetNaCl randombytes returns on /dev/urandom failure without filling the buffer. crypto_ed25519_keypair probes first, so the public API is mostly fail-closed, but a later callback failure still leaves uninitialized key material. Fail closed on RNG error.
  • --version / /status / GATEWAY_VERSION still print 0.2.0 while SHELLCLAW_RELEASE_VERSION is 1.0.0.
  • CHANGELOG.md claims a 1 req/s camera snapshot rate limit. docs/SECURITY.md says that throttle was removed until v1.2. Align them.
  • .github/workflows/publish-manifest.yml uploads the entire docs/ tree to Pages (SECURITY.md, IssueOps, fixtures). Stage a directory that contains only manifest.json and .nojekyll. Tag publishes also mint a fresh signing key via a temp SHELLCLAW_HOME; marketplace consumers cannot pin a stable publisher pubkey.
  • scripts/download_model.sh treats EXPECTED_SHA256 as optional and ships no default digest. HTTPS URL allowlisting is good; add pinned SHA256 for phi3 and tinyllama.
  • I2C tools cap addr 0x03-0x77 and len 1-256; hardware_i2c_read / write do not (scan already uses I2C_ADDR_MIN/MAX). Enforce the same limits in the backend.
  • Camera default_spawn uses unbounded waitpid (hardware_camera.c:190). A hung gst-launch-1.0 / libcamera-still blocks the process.
  • Raspberry Pi pin 26 (GPIO7 / SPI0 CE1) is RPI_HDR_GPIO while pin 24 CE0 is SFIO.
  • systemd/llama-server.service: quote $MODEL; add NoNewPrivileges / PrivateTmp / ProtectSystem=strict. Pin publish-manifest Actions to full SHAs.

Nice to Have

  • Jetson on-device sign-off stays deferred as documented. Do not block this PR on physical hardware once the Must Fix items are in.
  • config.c 1261 lines and the tool_X_set_config setter-global convention are already waived to v1.0.1 in AGENTS.md. Fine.
  • src/asap/manifest_build.c (620) and tests/test_manifest_keys.c (809) are over the 500-line house rule; the split from a single manifest file already happened.
  • Validate gpio_write value ∈ {0,1} in C, not only in the JSON schema enum (int_to_line_value treats any non-zero as HIGH).
  • Camera hang/timeout test with a mock spawn; ASan regression for gpio_mode JSON execute; a registry test that uses the real core getters and max_count == 8 (or whatever the new cap is).

Positive Highlights

  • SFIO rejection, pin-range checks, GPIO mutex, and a read-only snapshot path that does not reconfigure outputs for the Web UI.
  • Camera spawn is fixed argv + execvp (no shell), with path character allowlisting, .. rejection, JPEG magic check, and temp unlink on internal errors.
  • Signing path is serious: 0600 keys, atomic write + fsync, O_NOFOLLOW on read, pub/priv consistency, eager key load at gateway start (not on the first unauthenticated manifest GET), JCS goldens vs the Python verifier, and publish-manifest verify_manifest hard-fail (no || echo).
  • /api/hardware/* sits behind the central Bearer gate in http_lws.c (requires_auth treats every /api/ path as protected). v1.2 sensor/camera HTTP capture is honestly stubbed.
  • Release docs (JETSON_SIGNOFF.md, SECURITY.md, HARDWARE_SAFETY.md) match the deferred scope instead of claiming on-device numbers that were never measured.

Linux CI is green. The Must Fix items are logic bugs, not CI flakes. Do not merge until the tool table, gpio_mode UAF, and line-request lifetime are fixed.

Open in Web View Automation 

Sent by Cursor Automation: Adrianno’s personal code review

Comment thread src/tools/registry.c
out[n++] = ctx;
if (n < max_count && asap_invoke)
out[n++] = asap_invoke;
n += tool_hardware_get_all(out + n, max_count - n);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must Fix: this append is silently truncated.

tools_init() calls tool_get_all(g_tools, MAX_TOOLS) with MAX_TOOLS still 8 (src/core/bootstrap.c:24). The six core getters always return non-NULL (shell, web_search, file, cron, context, asap_invoke), so max_count - n is 2. Only gpio_read and gpio_write are copied. gpio_mode, i2c_read, i2c_write, i2c_scan, and camera_capture never reach the agent, even though hardware_enabled defaults to 1 (config.c:942).

Fix both caps together: raise MAX_TOOLS and agent_tool_t flat_tools[8] in src/core/dispatch.c:31 (that file is not in this diff; widening bootstrap alone overflows flat_tools when tool_count > 8). Need at least 13 slots (6 core + 7 hardware).

Add a test that uses the real core getters (not NULL stubs) and asserts the live tool list contains the hardware names when hardware is enabled.

Comment thread tests/test_registry.c
FILE *f;
config_t *cfg = NULL;
char errbuf[256];
const tool_t *tools[32];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test cannot catch the production cap.

Core getters are stubbed to NULL above, and the buffer is 32 slots. tool_get_all therefore sees n == 0 and copies all seven hardware tools. In the real binary those getters are never NULL, so MAX_TOOLS 8 drops five of them.

Keep the schema checks, but add a path that either links the real tools or asserts tool_get_all(buf, 8) with non-NULL core stubs and expects hardware names still present (after the cap is raised).

Comment thread src/tools/hardware_tools_gpio.c Outdated
hw_tools_json_error(result_buf, max_len, errbuf);
return -1;
}
snprintf(result_buf, max_len, "{\"pin\":%d,\"mode\":\"%s\"}", pin, mode);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must Fix: use-after-free on mode.

mode points at mode_item->valuestring (line 134). cJSON_Delete(root) at line 140 frees that string. hardware_gpio_mode(pin, mode, …) and this success snprintf then read freed memory. gpio_read / gpio_write copy ints before delete; this path does not.

Copy into a stack buffer (char mode_buf[8]) before cJSON_Delete. Add an execute-path test in tests/test_hardware_tools.c (ASan will catch this today if you call HW_TOOLS_GPIO_MODE.execute). tests/test_hardware_tools.c never calls gpio_mode. This is latent until the 8-slot tool cap is raised, then it is on the live gpio_mode path.

Comment thread src/hardware/hardware_libgpiod.c Outdated
}
ret = 0;
done:
release_request(request);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must Fix: the driven level does not persist.

gpiod_line_request_release returns the line to the kernel. On gpio-cdev that typically clears FLAG_IS_OUT and the pin goes back to the controller default (input / Hi-Z). gpioset holds the request for this reason. The same release_request pattern is in hardware_gpio_mode at line 425.

gpio_write is one of the two hardware tools that currently register (MAX_TOOLS 8), so this is not a dormant path. The agent reports {"pin":N,"value":1} after a pulse, not a held HIGH.

Keep a per-pin gpiod_line_request * for the process lifetime (mutex you already have). Release on gpio_mode input, hardware shutdown, or process exit. The snapshot path that avoids requesting outputs can stay as-is.

set_err(errbuf, errbufsz, "camera: unsafe output path");
return -1;
}
snprintf(out_path, out_pathsz, "%s", output_path);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Fix: path_chars_safe() allows /, so an LLM-supplied absolute path such as /home/user/.shellclaw/config.toml or …/keys/ed25519.priv is accepted and overwritten with JPEG bytes. That bypasses the workspace_only bound write_file enforces.

.. and shell metacharacters are blocked (good), but there is no workspace / capture-dir containment. Once MAX_TOOLS is raised, camera_capture becomes a write-anywhere primitive for anything the agent uid can write.

Reject paths outside a dedicated capture directory (or the file-tool workspace). Prefer ignoring LLM path in v1.0 and always using make_temp_output().

Comment thread src/asap/manifest_keys.c Outdated
g_manifest_keys_test_fail_backup_write = 0;
return -1;
}
fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, MANIFEST_KEY_FILE_MODE);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Fix: writes are missing O_NOFOLLOW.

Reads at line 211 correctly use O_RDONLY | O_NOFOLLOW. This open(O_WRONLY|O_CREAT|O_TRUNC) follows a planted ed25519.priv.tmp / .bak.* symlink and would truncate the target, then write private-key bytes into it.

Add O_NOFOLLOW, fstat the fd, require a regular file, and re-check (st_mode & 0077) == 0 on that fd rather than trusting the earlier lstat. Cover symlink-on-write in tests/test_manifest_keys.c (read-symlink is already tested).

Comment thread src/core/main.c Outdated
@@ -112,6 +115,20 @@ static int parse_args(int argc, char **argv, const char **config_path_out)
printf("%s\n", VERSION);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Fix: --version still prints VERSION (0.2.0 at line 35 of this file). Manifests and src/core/version.h advertise SHELLCLAW_RELEASE_VERSION 1.0.0. /status in src/core/dispatch.c and GATEWAY_VERSION in src/gateway/http_lws.h have the same stale 0.2.0.

Wire CLI, /status, and the gateway version string to SHELLCLAW_RELEASE_VERSION so a v1.0.0 tag and shellclaw --version cannot disagree.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Left a non-blocking comment. Cursor Security Agent: Security Reviewer did not reach a terminal state within 8 minutes, and an unresolved HIGH Agentic Security Review finding remains on camera_capture path handling, so this is not approved. Assigned adriannoes for human review; Bugbot was not present on this PR.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

MAX_TOOLS 8 dropped gpio_mode, i2c_*, and camera_capture after the six
core tools. Share SHELLCLAW_MAX_TOOLS (16) across bootstrap, dispatch,
and the registry so the full hardware table reaches the LLM.

Refs: #83
gpio_mode_exec aliased cJSON valuestring and kept reading it after
cJSON_Delete. Copy mode into a stack buffer first, and reject gpio_write
values outside {0,1} with the offending integer.

Refs: #83
Releasing the request after gpio_write/gpio_mode made the pin a pulse
instead of a held level. Keep per-pin requests for output, release on
input mode or shutdown, and cover the lifetime with a fake-line test.

Refs: #83
Caller-supplied JPEG paths only blocked .. and metacharacters, so an
absolute path could overwrite files outside the agent workspace. Reject
those paths with the same realpath walk as write_file, and bound hung
CLI spawn with a waitpid timeout.

Refs: #83
Reject reserved 7-bit addresses outside 0x03-0x77 and payloads over 256
bytes in the I2C backend so tool JSON cannot drive invalid kernel xfers.

Refs: #83
TweetNaCl randombytes previously returned without filling the buffer on
CSPRNG failure. Zero the output and abort so keygen cannot proceed with
uninitialized bytes.

Refs: #83
Open ed25519 key temps with O_NOFOLLOW and re-check fstat mode after
fchmod so a planted symlink cannot redirect a newly minted private key.

Refs: #83
--version and /status still printed 0.2.0 after the 1.0.0 release
string lived in version.h. Use SHELLCLAW_RELEASE_VERSION in both paths.

Refs: #83
The 1.0.0 notes claimed a 1 req/s snapshot throttle that SECURITY.md
documents as deferred until Phase 7 HTTP capture.

Refs: #83
Tag publish uploaded the whole docs/ tree and minted a fresh Ed25519
key per run. Stage pages-dist with manifest.json only and require the
stable publisher key secrets.

Refs: #83
Do not vendor a guessed GGUF digest. Document EXPECTED_SHA256 plus
optional PHI3_SHA256 / TINYLLAMA_SHA256 from the HF file metadata page.

Refs: #83
Header pin 26 is GPIO7 / SPI0 CE1, same special-function class as CE0
on pin 24. Keep gpio_write from claiming the chip-select line.

Refs: #83
Unquoted $MODEL split on spaces, and the unit lacked sandbox directives.
Quote EnvironmentFile expansions and add NoNewPrivileges, PrivateTmp,
and ProtectSystem=strict.

Refs: #83
make clean skipped gpio snapshot, tegrastats, libgpiod, and tests/stubs
.o files, so a Docker ELF leftover could break a later macOS link.

Refs: #83
@adriannoes

Copy link
Copy Markdown
Collaborator Author

Review-fix follow-up (e76f0205cca5d3)

Pushed 14 atomic commits to development. Not merging. Local gate: CI=true make test green on macOS (libgpiod hold tests are compile-only smoke here; Linux CI exercises the fake-line path).

Must Fix

  • Tool table cap: SHELLCLAW_MAX_TOOLS is 16 in tool.h; bootstrap + dispatch no longer truncate after 8 slots. Registry/dispatch tests cover the full hardware names and a 13-tool forward.
  • gpio_mode UAF: mode string is copied before cJSON_Delete. gpio_write also rejects value outside {0,1}.
  • GPIO line-request lifetime: write / mode-output hold the libgpiod request; mode-input and shutdown release. Fake-line seam is always compiled so test_hardware_libgpiod can see it without a chip.
  • Cursor Security HIGH (camera_capture path): LLM path is rejected unless it resolves under the agent workspace (same realpath + parent walk as write_file). Checked at the tool boundary before camera availability, and again in hardware_camera_capture. Auto temp /tmp/shellclaw_cam_XXXXXX.jpg stays allowed. Tests: backend + test_hardware_tools outside-workspace reject.

Should Fix

  • I2C backend: addr 0x03–0x77, len 1–256.
  • Camera default spawn: bounded waitpid (hung CLI cannot stall the agent).
  • TweetNaCl randombytes: fail-closed (memset + abort) when /dev/urandom fails.
  • Manifest key write: O_NOFOLLOW + post-fchmod fstat (regular file, mode 0077 == 0).
  • --version / /status now print SHELLCLAW_RELEASE_VERSION (1.0.0). GATEWAY_VERSION is still 0.2.0 (out of this slice).
  • CHANGELOG camera 1 req/s claim aligned with docs/SECURITY.md (deferred to Phase 7 HTTP capture).
  • Tag publish stages pages-dist/ (manifest.json + .nojekyll only) and fail-closes without secrets SHELLCLAW_MANIFEST_ED25519_PRIV / _PUB. PR validate-manifest may still mint a temp key.
  • download_model.sh: no vendored digest; document EXPECTED_SHA256 / PHI3_SHA256 / TINYLLAMA_SHA256.
  • RPi header pin 26 marked SFIO (GPIO7_CE1).
  • llama-server.service: quoted env args + NoNewPrivileges / PrivateTmp / ProtectSystem=strict.
  • make clean now removes leftover hardware + tests/stubs .o files (Docker ELF leftovers broke macOS link).

Intentionally skipped

  • Pinning GitHub Actions SHAs.
  • Inventing GGUF SHA256 values.
  • Splitting hardware_camera.c (~594) and hardware_libgpiod.c (~546) under the 500-line house rule (follow-up).
  • Jetson on-device GPIO/I2C/llama-server smoke — known pending, not a merge gate.

Please re-run Cursor Security on 5cca5d3 for the camera path finding.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a non-blocking comment. Cursor Security Agent: Security Reviewer did not reach a terminal state within 8 minutes, and an unresolved HIGH Agentic Security Review finding remains on camera_capture path handling, so this is not approved. Assigned adriannoes for human review; Bugbot was not present on this PR.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant