fix(phase5-slice05): pre-merge fixes — C1/JCS/SIG/TOCTOU blockers + medium fixes#43
Merged
Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Repository context
ShellClaw is a C99/C11 edge AI agent (Jetson / Raspberry Pi) with a modular src/ layout (core, providers, tools, channels, gateway, asap, sandbox, hardware, crypto). Build/test is Makefile-driven (make shellclaw, CI=true make test); gateway tests need libwebsockets. Thread-safety: gateway worker threads must hold agent_lock() around agent_run().
PR context
Phase 5 slice 05 pre-merge hardening: closes audit blockers (C1 double-free, JCS RFC 8785 compliance, SIG upstream verification, TOCTOU key reads), medium fixes (camera, cron toggle, I2C comment), sanitizer/CI hardening, and documents known v1.0.1 debt in AGENTS.md. Touches asap/, crypto/, gateway/, hardware/, CI workflow, and ~900 lines of new tests.
Summary
High-quality, security-conscious patch with strong regression tests (JCS golden vectors, upstream canonical byte pin, key TOCTOU/symlink guards, cron toggle fix, C1 alloc-failure sweep). The core C fixes look correct and well-motivated. One CI workflow issue undermines the stated SIG verification gate and should be fixed before merge.
Must Fix
1. publish-manifest.yml — validation failures can leave the job green
- Pydantic path (lines 62–71):
python3 -c "…Manifest" && { … model_validate … } || echo "skipped"— if import succeeds butmodel_validateraises, bash falls through to|| echoand the step exits 0. verify_manifestpath (lines 72–86):assert ok is Trueruns inside anif …; thenbranch. Under bashset -e, failures insideif/thendo not abort the step — a rejected signature also exits 0.
For a PR whose acceptance criteria include upstream verify_manifest: True, both paths must hard-fail on validation/signature rejection. Replace the || echo fallback and move verification out of a non-failing if branch (e.g. python3 -c "…; sys.exit(0 if ok else 1)" after pip install asap-protocol==2.5.0).
Should Fix
- Pin
asap-protocolin CI (publish-manifest.yml:61,:126) — unpinned install can drift from theEXPECTED_STUB_NULL_CANONICALgolden (documented against 2.5.0). - Skills truncation is silent (
manifest_build.c:126–128) — manifests with >64 skills log to stderr only; marketplace consumers may see an incomplete capability list. Fail the build or sort deterministically. verify_manifest.shSKIP exits 0 (verify_manifest.sh:47–50) — safe for optional local use, but do not wire into release gates without guaranteeing the package is installed.- Rotation edge case (
manifest_keys.c:548–551) — if pub write fails and priv restore also fails, disk can hold mismatched new-priv/old-pub until manual repair. Rare, but consider restoring from.bak.<ts>. - No gateway bootstrap key-failure test —
bootstrap.c:273–282aborts startup on key errors; add an integration test that expects gateway init failure when keys dir is unwritable.
Nice to Have
test_manifest_keys_rotate_prunes_backupssleeps 6s (test_manifest_keys.c:724–725) — consider a test hook for backup timestamps.- Deduplicate workflow Python with
scripts/verify_manifest.sh(once the script hard-fails on rejection). EXPECTED_STUB_NULL_CANONICALpinsversion":"1.0.0"— any semver bump requires updating the golden string (intentional but brittle).
Positive Highlights
- Upstream SIG path A is validated end-to-end: default-populated manifest tree + 624-byte canonical golden (
test_manifest_build.c:594–637) gives high confidence JCS output matchesasap.crypto.signing.canonicalize. - Key security is thorough:
lstatfor priv perms,O_NOFOLLOW+fstaton read, symlink regression test, atomic writes, bounded backup pruning, pub/priv consistency (M1). - DoS fix moves key creation off the unauthenticated manifest route to gateway bootstrap (
bootstrap.c:266–283,routes.c:624–634). - Cron toggle off-by-one fixed with an exact-length guard and HTTP regression test (
routes.c:819–824,test_gateway_http.c:726–741). - C1 double-free regression via
cJSON_InitHooksalloc sweep (test_manifest_build.c:367–436).
Sent by Cursor Automation: Adrianno’s personal code review
Resolve the four release-blocking defects from the second-opinion audit plus the signing-key hardening and a class of orphan-node cJSON leaks, so the SignedManifest verifies with the upstream reference verifier asap.crypto.signing.verify_manifest. Blockers: - C1 (manifest_build.c): stop double-freeing the cJSON parent in cjson_add_string_to_object_checked on alloc failure; let manifest_build_tree's cJSON_Delete(root) clean up the still-attached child. - JCS (jcs.c): replace %.17g with RFC 8785 shortest round-trip number serialization matching the reference jcs 0.2.1 (Number.prototype.toString): exponent notation only when |x|>=1e21 or |x|<1e-7, + on positive exponents, 1e20 -> 100000000000000000000. Byte-exact tests added. - SIG path A (manifest_build.c): emit the full default-populated manifest (supported_versions, ttl_seconds, auth/sla/verification=null, endpoints.events=null, input_schema/output_schema/throughput_tokens_per_second=null) and use the field name class_ (not the alias class) so the C-signed bytes are byte-identical to asap.crypto.signing.canonicalize. SignedManifest now verifies with the upstream verifier (verified end-to-end on Ubuntu CI). - TOCTOU (manifest_keys.c): open(O_RDONLY|O_NOFOLLOW) + fstat(fd) closes the stat->open race; lstat perm check rejects symlinked priv keys. Symlink regression test added. Key hardening: - M1: rotation restores old_pub to memory on pub-write failure and checks pub/priv consistency on load (pub == priv seed||pub layout). - M2: prune .bak.<ts> backups to the 5 most-recent per key after rotation. - M5: create the Ed25519 keypair at gateway bootstrap (init_subsystems) instead of lazily on the first unauthenticated /.well-known/asap/manifest.json hit; CLI without gateway still starts without keys. Orphan-leak class (caught by LeakSanitizer on Linux CI): cJSON_AddItemToObject duplicates the key via cJSON_strdup (cJSON.c:2094) using the malloc hook, so under OOM the key dup can fail even after the value item was allocated. 34 sites in manifest_build.c ignored the return, leaving already-allocated items unattached -- orphaned -- so the final cJSON_Delete(root) never reached them. Add cjson_add_item_to_object_checked / cjson_add_item_to_array_checked helpers that check the return and cJSON_Delete the orphan on failure; route all 34 sites through them. The success-path manifest bytes are unchanged (byte-exact upstream-canonical test still passes). CI gate (publish-manifest.yml): install asap-protocol (not the unrelated asap ASP/clingo package) via setup-python 3.13, pin asap-protocol==2.5.0, and run Pydantic + upstream verify_manifest unconditionally with sys.exit(0 if ok else 1) so a rejected signature hard-fails the job (the prior || echo / if-then patterns masked validation failures). scripts/verify_manifest.sh wraps the same round-trip with a --strict mode that fails when the package is absent. Tests: C1 alloc-failure sweep (budgets 0..47, ASan clean), JCS byte-exact table, TOCTOU symlink, rotation consistency, backup pruning, manifest-level tamper detection (flip-byte / swapped-pub reject), upstream-canonical byte pin.
- M3 (hardware_camera.c): pass quality to all three capture builders -- nvjpegenc quality=<n>, libcamera-still --quality <n>, USB UVC MJPG documented as firmware-controlled (ignored). quality is no longer silently dropped. - CAM (hardware_camera.c): unlink the auto-generated temp .jpg on every internal error path (spawn failure, invalid JPEG) when the module owns the path; caller-supplied and success paths are untouched (caller/consumer owns those). - M4 (routes.c): fix the /api/cron/<id>/toggle length guard (suffix+8 -> suffix+7, exact match) so POST /toggle no longer falls through to the DELETE branch and 405. Trailing garbage like /toggle/extra is also rejected. HTTP-level test added. - M0 (crypto.c): guard the zero-length memcmp in crypto_ed25519_verify so a NULL pointer with message_len==0 (a valid empty-message verify input) is not passed to memcmp (strict-reading UB). Empty-message null-pointer test added. - M0b (hardware_i2c.c): document why probe_address ignores addr (the slave is already selected via I2C_SLAVE in set_slave before probing). Note: the cron-toggle URL buffers in test_gateway_http.c are sized to 512 bytes to satisfy GCC -Wformat-truncation (g_base_url[128] + id[128] + path > 256).
…format-truncation - H3: add -fwrapv to TWEETNACL_CFLAGS so the vendored TweetNaCl 20140427 signed left-shifts (tweetnacl.c:281,685) are well-defined wraparound, silencing the UBSan left-shift-of-negative warnings. README notes the expected upstream behavior. - SAN: add -fno-sanitize-recover=all to SANITIZE_CFLAGS and UBSAN_OPTIONS/ASAN_OPTIONS=halt_on_error=1:abort_on_error=1 to the test-sanitize recipe so real undefined behavior fails CI instead of being printed-and-recovered. Suppress the known vendored cJSON NaN->int float-cast UB (cJSON.c:2519, triggered by test_jcs cJSON_CreateNumber(NAN)) via a targeted -fno-sanitize=float-cast-overflow on CJSON_CFLAGS only -- ShellClaw src/ retains full UBSan coverage. (macOS Clang does not emit -Wformat-truncation; the jcs.c exponent buffer and test_gateway_http.c URL buffers were widened in their respective commits to satisfy GCC -Wformat=2 -Werror on Ubuntu CI.)
Record the two structural items deferred to v1.0.1 as Known debt in AGENTS.md: - B1: src/core/config.c is 1261 lines (single-struct TOML parser); a clean hardware-only extract leaves it at ~1094, still over 1k. A two-section extract (config_hardware.c + config_asap.c -> ~930) is scheduled for v1.0.1. Waived for v1.0.0 (the asap_skill_descriptions table is ASAP-section, not hardware). - H1: g_hw_cfg in hardware_tools_helpers.c is one instance of the project-wide tool_X_set_config setter-global convention (shell/file/web_search/asap_invoke/ context/hardware). A whole-convention refactor (pass const config_t *cfg through tool_t.execute / agent_tool_t.execute for all tools) exceeds the 10-file/300-line PR threshold (vtables + 13 callbacks + ~30 test sites); scheduled for v1.0.1. TODO-ref(phase5-slice05-H1) added at the declaration.
adriannoes
force-pushed
the
phase5/slice05-pre-merge-fixes
branch
from
June 25, 2026 16:31
fc4971e to
fff78d9
Compare
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
Phase 5 Slice 05 — the fix patch landing on
developmentbefore the v1.0.0 merge intomain. Closes every blocker and high/medium finding from the two-pass second-opinion audit (verdict escalated from READY-WITH-BLOCKERS to NOT READY). All four release-blocking defects (C1 double-free, SIG upstream-verify rejection, JCS non-compliance, TOCTOU) are resolved, plus the structural items (B1 waived, H1 path B debt note), medium fixes (M0–M5, CAM), and the re-run gate hardening (H3, SAN).SIG path A taken: the served manifest is now the upstream-canonical form (default-populated,
class_field name), so the dumpedSignedManifestverifies with the upstream reference verifierasap.crypto.signing.verify_manifest(verified end-to-end:verify_manifest: True). A CI step inpublish-manifest.ymlruns this and fails the workflow on rejection.What changed (by commit)
fix(asap,crypto): close v1.0.0 release blockers C1/JCS/SIG/TOCTOU + key hardening— C1 double-free inmanifest_build.chelper; JCS%.17g→RFC 8785 shortest round-trip (jcs.c); SIG path A default-populated manifest +class_; TOCTOUopen(O_NOFOLLOW)+fstat+lstat; M1 rotation pub/priv consistency; M2 backup pruning (keep 5); M5 keypair creation at gateway bootstrap;publish-manifest.ymlasap-protocol+setup-python 3.13+verify_manifeststep; newscripts/verify_manifest.sh; tamper-detection test.fix(hardware,gateway,crypto): medium fixes M0/M3/M4/CAM/M0b— M3 camera quality applied to nvjpegenc/libcamera (USB documented as firmware-controlled); CAM temp.jpgunlink on internal error paths; M4 cron toggle off-by-one (suffix+8→suffix+7, exact match); M0 zero-lengthmemcmpguard; M0bprobe_addresscomment.build(ci,sanitizer): harden test-sanitize and suppress vendored UB— H3-fwrapvinTWEETNACL_CFLAGS; SAN-fno-sanitize-recover=all+halt_on_error=1:abort_on_error=1in thetest-sanitizerecipe; targeted-fno-sanitize=float-cast-overflowonCJSON_CFLAGSfor the known vendored cJSON NaN→int cast UB.docs(agents): record config.c 1k waiver and g_hw_cfg setter-global debt— B1 waiver + H1 path BTODO-ref+AGENTS.mdKnown debt (both scheduled for v1.0.1).Verification (local, macOS — CI runs Ubuntu)
All run on a clean single invocation:
CI=true GATEWAY=1 make test— 56 test binaries green, zero regressionsCI=true GATEWAY=1 LIBGPIOD=0 make test— stub GPIO fallback greenmake static(cppcheck) — zero findings (fixed onevariableScopelint injcs.c)make test-sanitize(hardened, abort-on-error) — green, zero UBSan/ASan findings in ShellClaw sourceGATEWAY=1 make coverage— core 84.3% (≥ 80% gate)make release— 1,247,816 bytes (< 2 MB CI gate; over the < 600 KB PRD budget but pre-existing ondevelopmentand fits on the Jetson target — waived for v1.0.0)validate-manifestjob simulated withasap-protocol2.5.0:Pydantic OK+upstream verify_manifest: True(exit 0); YAML parses,bash -non all run blocksGates that require a Linux host / Jetson (not run locally, not a code defect)
gpio-mockupis a Linux kernel module not available on macOS, and Docker on macOS cannot load it either (the LinuxKit VM kernel doesn't ship it — recompiling that kernel is a non-reproducible hack). Must run on a bare Linux host or the Jetson before tagging; folded into the Phase B on-device sign-off../scripts/ci-local.sh— same platform constraint (Ubuntu 24.04+ CI mirror; invokes the gpio-mockup ritual).publish-manifest.ymlverify_manifestCI step — runs on GitHub Actions (Ubuntu); the workflow is updated and simulated green locally, but the step only truly runs on push.Test plan
cijob (static, test with/without libgpiod, test-sanitize hardened, release < 2 MB, coverage ≥ 80%) — greenvalidate-manifestjob —Pydantic OK+upstream verify_manifest: True./scripts/ci-local.shgreenNotes
asapPyPI package is an unrelated ASP/clingo project; the workflow now installsasap-protocol(the real ASAP Protocol SDK) and pins Python 3.13 viaactions/setup-python@5.auth/sla/verification=null,ttl_seconds=300,supported_versions=["2.2"],endpoints.events=null,class_instead ofclass, etc.). This is acceptable because/.well-known/asap/manifest.jsonis new in v1.0.0 (no external consumers yet) and the new shape is exactly what the upstreamManifestPydantic model (extra=forbid) accepts..cursor/dev-planning/(gitignored) and are not part of this PR.