Skip to content

Build deterministic physical HID fixture and capture lab - #1238

Draft
malpern wants to merge 94 commits into
masterfrom
codex/pico-hid-fixture
Draft

Build deterministic physical HID fixture and capture lab#1238
malpern wants to merge 94 commits into
masterfrom
codex/pico-hid-fixture

Conversation

@malpern

@malpern malpern commented Jul 28, 2026

Copy link
Copy Markdown
Owner

What changed

  • adds the ESP32-S3/Pico physical USB HID fixture, secure Wi-Fi control plane, dual-slot OTA, display health, animated device UI, safety releases, and QEMU coverage
  • adds the native KeyPath HID Capture Jig with a fail-closed focus oracle, exact event ledger, resource admission, responsive UI, and persisted JSON evidence
  • adds combined physical runners plus a Shift lead/hold/release diagnostic matrix correlated with bounded firmware traces
  • expands the installer/runtime lab, scenario campaign tooling, managed clone support, and lab documentation
  • routes CLI start/stop/restart service operations through the privileged helper instead of invoking system launchd control from an unprivileged process

Why

KeyPath needs repeatable end-to-end evidence that real installer/runtime builds correctly process physical keyboard input, including high-rate and resource-pressure regressions. Previous TextEdit and scheduler-only checks could not prove focus, exact output, or safe release. The new Jig and fixture preserve independent machine-readable evidence and fail closed when focus or host readiness is invalid.

Validation

  • Scripts/lab/pico-hid-fixture-tool test (host/core/client/runner/matrix plus ESP32-S3 QEMU smoke)
  • native HID Capture Jig suite: 26 tests
  • KeyPath privileged-helper and runtime-health regression suite: 56 focused tests
  • production firmware build and authenticated OTA verification on Waveshare ESP32-S3 build 4d1cb1cb54c4
  • connected-board status: valid OTA slot, USB mounted, Wi-Fi connected, splash complete, display heartbeat healthy
  • admitted three-cell physical Shift smoke matrix: baseline, 4 ms lead, and 8 ms lead all delivered exact 24/24 output with valid focus, released keys/modifiers, complete firmware traces, and zero late reports
  • full 5x5 physical Shift lead/release matrix: 25/25 valid cells, 1,500/1,500 characters captured, 4,000 firmware trace reports, valid focus and clean final releases throughout, and zero late reports
  • full-matrix diagnosis: 10 Shift demotions were confined to four of five zero-lead cells; all 20 cells and 1,200 characters with a 2 ms-or-greater modifier lead were exact

Pending physical acceptance

This remains a draft until the remaining person-dependent physical gates are complete. Real USB HID input cannot target a background app safely. Remaining gates are:

  • final visual confirmation that the repaired splash completes
  • USB disconnect/reconnect and intentional Wi-Fi-loss safe-abort checks

Excluded focus-, memory-, load-, and network-admission attempts are documented as harness admissions, not KeyPath outcomes.

malpern added 30 commits July 24, 2026 20:14
malpern added 2 commits July 28, 2026 06:50
# Conflicts:
#	Sources/KeyPathAppKit/CLI/SystemFacade.swift
#	Sources/KeyPathAppKit/Core/HelperManager+RequestHandlers.swift
#	Sources/KeyPathAppKit/Core/HelperProtocol.swift
#	Sources/KeyPathCore/KeyPathHelperContract.swift
#	Sources/KeyPathHelper/HelperProtocol.swift
#	Sources/KeyPathHelper/HelperService.swift
#	Sources/KeyPathHelper/Info.plist
#	Tests/KeyPathTests/CLI/CLIServiceTests.swift
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review notes on the supplied diff (truncated at 60000 bytes, so I could not see the actual changes to Sources/KeyPathCLI/Core/PrivilegedOperationsRouter.swift, Commands/System/SystemUninstallCommand.swift, Sources/KeyPathCLISupport/CLIReportModels.swift, Core/ServiceBootstrapper.swift, or the related test updates. Those are the only non-lab-tooling production changes in this PR (privileged ops / uninstall path) and deserve an explicit dedicated review pass — please make sure they got scrutiny separately from the huge lab-infra diff.

Within what was visible:

  1. Scripts/build-and-sign.sh: the new SKIP_DEPLOY branch wraps the existing stop/replace/restart block in if ... else ... fi, with the closing fi added right before the website-publishing section. That block spans a lot of pre-existing logic (likely with its own nested if/fis). Worth running bash -n Scripts/build-and-sign.sh to confirm the nesting is still balanced — an off-by-one fi here would silently skip or always-run the website publish step.

  2. Scripts/lab/hid-capture-jig-client: the file-RPC (command.json / response.json) is a single shared pair with no lock. If scenario-matrix-runner or similar ever invokes multiple hid-capture-jig-client commands concurrently, a second arm/status call could overwrite command.json before the jig processes the first, and the UUID match wouldn't prevent cross-talk, just detect it after the fact via timeout. Low risk if usage stays strictly serial, but worth a comment/guard if that assumption could change.

  3. Scripts/lab/desktop-bootstrap: pins sha256 checksums (good), but downloads Peekaboo/mcporter from github.com/openclaw/... — AGENTS.md elsewhere references steipete/tap/peekaboo and steipete/tap/mcporter. Please confirm openclaw is the intended upstream publisher for these binaries; a pinned checksum only helps if the source repo itself is the right one.

No other functional issues found in the portions reviewed (CaptureSession/KeycapBurstModel state machines, assert-runtime-state/assert-uninstalled-state logic looked internally consistent).

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Note: the supplied diff (133 files, +16411/-183) was truncated before reaching several files — including the Swift production changes (PrivilegedOperationsRouter.swift, SystemUninstallCommand.swift, CLIReportModels.swift, ServiceBootstrapper.swift, PostconditionLintTests.swift) and most of the lab tooling (HIDCaptureJig/main.swift, physical-hid-capture-run, pico-hid-fixture-client/tool, remote.sh, scenario-matrix-runner, installer-scenario, etc.). The findings below only cover what was visible in the truncated diff.

  1. Supply-chain source changeScripts/lab/desktop-bootstrap now downloads Peekaboo and mcporter release tarballs directly from github.com/openclaw/Peekaboo and github.com/openclaw/mcporter, rather than the steipete/tap Homebrew formulas this repo's docs/scripts otherwise reference for these tools. The pinned SHA256 checksums protect against in-transit tampering, but please confirm openclaw is actually the intended/authoritative publisher before this runs unattended as part of lab bootstrap — if it's a typo'd org or an unverified fork, the checksum pin will just keep validating against the wrong artifact indefinitely.

  2. Scripts/build-and-sign.sh SKIP_DEPLOY wrap — the new if [ "${SKIP_DEPLOY:-0}" = "1" ]; then ... else ... fi wraps roughly 70 pre-existing lines (stop/replace/restart/verify). Placement looks correct from the diff context, but given the size of the wrapped region it's worth running bash -n Scripts/build-and-sign.sh / shellcheck to make sure there's no unbalanced nested if/fi inside before merging.

  3. Minor reliabilityScripts/lab/assert-runtime-state and assert-uninstalled-state call subprocess.run on external binaries (keypath-cli, launchctl, pgrep, systemextensionsctl, etc.) without catching FileNotFoundError. A missing/misconfigured binary path will surface as a raw Python traceback instead of the scripts' otherwise clean ... assertion: ... failure messages, which makes lab CI logs harder to scan.

No other correctness/security issues found in the visible portion.

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review notes (based on the supplied diff only; it was truncated at 60000 bytes, so remote.sh, keypath-lab, scenario-matrix-runner, the pico-hid-fixture C/ESP32 sources, and the PrivilegedOperationsRouter/SystemUninstallCommand Swift changes were not visible and are not covered below).

  1. Scripts/lab/hid-capture-jig-clientwait subcommand can report success for a failed capture.
    In main(), the early-exit path inside the polling loop returns based on snapshot.state (passed/failed), but if the loop times out it falls through to response = send("finalize") and then hits the shared tail:

    print_response(response)
    return 0 if response.get("ok") else 1

    That final return uses the RPC-level ok field, not the capture's pass/fail state. If finalize returns ok: true (the command was processed) while snapshot.state == "failed" (e.g. mismatched output, unreleased keys), wait will exit 0 even though the capture actually failed. Suggest checking response.get("snapshot", {}).get("state") == "passed" for the timeout/finalize path too, consistent with the in-loop branch.

  2. Scripts/build-and-sign.sh — new SKIP_DEPLOY if/else wraps a large pre-existing block; worth a syntax sanity check.
    The new if [ "${SKIP_DEPLOY:-0}" = "1" ]; then ... else opens well before the "Stop running KeyPath..." section and its matching +fi is inserted after the "Publish help content" comment header, spanning several pre-existing nested if/fi blocks that weren't shown as changed. The nesting looks balanced from the diff context, but given the size of the wrapped region, it'd be good to confirm with bash -n Scripts/build-and-sign.sh (and a SKIP_DEPLOY=1 dry run) before merge, since a mismatched fi here would break the normal (non-matrix) release path.

No other actionable correctness/security issues found in the portion of the diff reviewed (the Python assertion scripts — assert-runtime-state, assert-uninstalled-state — and desktop-bootstrap's checksum-verified tool installation look sound).

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Reviewed the supplied diff (note: the diff payload was truncated after ~60KB, so large portions — including Scripts/lab/remote.sh, scenario-matrix-runner, keypath-lab, and the Sources/KeyPathCLI*/ServiceBootstrapper changes — were not visible to this review and should get a follow-up pass).

Findings from the visible portion:

  1. Scripts/lab/desktop-bootstrap — consent-prompt handling changed from auto-approve to checkpoint-exit, worth confirming callers handle it. The old code detected the private-window-picker "Allow" button and clicked it via peekaboo click, letting bootstrap finish unattended. The new code only detects a screen-capture or private-window-picker dialog via JXA and immediately exit 4s with a desktop_bootstrap_checkpoint message — it no longer grants consent itself. If no orchestrator (e.g. keypath-lab/remote.sh, not visible in this diff) re-invokes desktop-bootstrap after clicking through the prompt, VM bootstrap will now hang/fail wherever it previously succeeded unattended. Worth double-checking the checkpoint is actually consumed somewhere.

  2. Scripts/lab/assets/peekaboo-lab-host-arm64 — binary blob checked in without a build step tying it to the adjacent .c source. desktop-bootstrap pins its SHA256 (PEEKABOO_HOST_SHA256) before use, which is good, but nothing in the diff recompiles peekaboo-lab-host.c to verify the checked-in binary actually matches that source. Consider adding a CI check (or at least a comment/script) that rebuilds and diffs the binary so the pinned hash can't silently drift from the source of truth.

  3. Scripts/build-and-sign.sh — website publish step now runs even when SKIP_DEPLOY=1. The new if/else/fi around the local-deploy block places the closing fi right before the "Publish help content to website" section, so a matrix/CI build that sets SKIP_DEPLOY=1 to avoid touching the operator's installed app will still attempt to publish to gh-pages when $GHPAGES_DIR exists and SKIP_WEBSITE/SKIP_NOTARIZE aren't set. If that's intentional (e.g. matrix runs never have the gh-pages worktree checked out) it's fine, but worth an explicit comment or SKIP_WEBSITE default for the matrix-build path to avoid an accidental publish from a non-canonical build.

No other correctness/security issues stood out in the portion of the diff reviewed (new Python/Swift lab assertion scripts, CaptureSession/KeycapBurstModel logic, and checksum-pinned tool downloads all looked internally consistent).

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Automated review of the supplied diff (note: the diff payload was truncated before reaching Sources/KeyPathCLI/** and Sources/KeyPathCLISupport/**, so PrivilegedOperationsRouter.swift, SystemUninstallCommand.swift, CLIReportModels.swift, ServiceBootstrapper.swift, and the related test changes were not visible for this pass — please get those specifically reviewed).

Findings from the visible portion (Scripts/lab/**, Scripts/build-and-sign.sh):

  1. Scripts/build-and-sign.shSKIP_DEPLOY=1 still publishes the help-content website. The new if [ "${SKIP_DEPLOY:-0}" = "1" ]; then ... else <stop/replace/restart KeyPath> fi block closes right before the # Publish help content to website section, so the GHPAGES_DIR/SKIP_WEBSITE publish step runs unconditionally regardless of SKIP_DEPLOY. If the intent of SKIP_DEPLOY is "produce the signed artifact only, don't touch anything else on this machine" (as the added comment implies — "must not replace or restart the operator's installed app"), publishing to the local gh-pages worktree during a matrix build seems like an unintended side effect. Worth confirming this is intentional, or gate the website publish on SKIP_DEPLOY too.

  2. Scripts/lab/damage-kanata-service isn't idempotent under set -euo pipefail. It unconditionally runs sudo -n /bin/launchctl bootout system "$plist" (or the label form) with no fallback if the job is already unloaded. launchctl bootout exits non-zero when there's nothing to boot out, and with set -e that aborts the whole fixture script instead of treating "already stopped" as success. If any scenario ever calls this fixture when Kanata isn't currently loaded, the script dies instead of confirming the degraded state.

  3. Scripts/lab/desktop-bootstraptrap ... EXIT set inside install_desktop_tools. The function does trap "/bin/rm -rf '$temp'" EXIT, which is a shell-global trap, not function-scoped. That's fine today since nothing else in the script sets an EXIT trap, but it's a fragile pattern if a future edit adds another cleanup trap elsewhere in this script — the later one silently wins and the earlier cleanup is lost.

Nothing else in the visible diff (the new HIDCaptureCore/CaptureSession Swift logic, assert-runtime-state, assert-uninstalled-state, hid-capture-jig-client) raised correctness concerns on read-through.

@malpern

malpern commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Latest reliability update (commit 592c9aa):

  • adds a two-step, compiled-in offline demo (top power, then touch) so Wi-Fi is not on the critical start path
  • caches the signed Jig app and adds hid-capture-jig-tool demo with in-Jig operator instructions
  • preserves an atomic inconclusive artifact and waits through the bounded capture window when control/abort is unconfirmed
  • adds ESP reset/heap/Wi-Fi/HTTP diagnostics, server restart recovery, no power-save, and a higher-priority HTTP task
  • adds a no-HID control soak that separates mDNS discovery from endpoint latency

Validation after merging current master:

  • fixture C core: pass
  • Python client/tool/campaign/runner/Shift suites: 41 tests pass
  • Swift HID Capture Jig: 26 tests pass
  • ESP32-S3 QEMU smoke: pass
  • production ESP-IDF build: pass (59% app partition free)
  • live pre-update control soak: 20/20, p95 206.745 ms, max 219.339 ms, no reset/build discontinuity

Local review gate selected the enforced remote claude-review; waiting for that check before merge. No physical HID was emitted during this update.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review notes (based on the supplied diff, which was truncated after ~60000 bytes — the actual Sources/KeyPathCLI/Core/PrivilegedOperationsRouter.swift, SystemUninstallCommand.swift, ServiceBootstrapper.swift, and the associated CLI test changes were not visible to me. Given this repo's own emphasis on postcondition-verified installer/uninstall mutations, that part of the diff deserves an explicit separate look before merge.)

Findings from the visible portion:

  1. Possible wrong upstream source for downloaded binaries (Scripts/lab/desktop-bootstrap): the new bootstrap fetches Peekaboo and mcporter directly from GitHub releases at github.com/openclaw/Peekaboo and github.com/openclaw/mcporter, whereas the code being replaced installed them from the steipete/tap/peekaboo and steipete/tap/mcporter Homebrew taps. The pinned SHA-256 checksums make tampering-in-transit unlikely, but if openclaw isn't actually the intended/trusted publisher for these tools (vs. a typo, fork, or unrelated org), this silently changes the trust root for two binaries that get code-signed as a background LaunchAgent-launched app and executed on every lab VM. Worth a quick confirmation that this is the intended source before merge.

  2. Behavior change in consent-prompt handling (Scripts/lab/desktop-bootstrap): the old code detected the Peekaboo "Allow" consent sheet and automatically clicked it. The new code only detects the prompt (via an osascript -l JavaScript check) and exits with code 4 plus a capture-consent-checkpoint.json artifact — it no longer clicks anything. That's a reasonable move if Apple's TCC path requires human-driven consent, but it means any orchestrator (keypath-lab, remote.sh, scenario runners) calling desktop-bootstrap must now explicitly special-case exit code 4 as "waiting for a human," rather than treating any non-zero exit as a hard failure. Please confirm the callers were actually updated to handle this new exit code (not visible in the truncated diff) rather than just failing the scenario.

No other correctness/security issues stood out in the visible Python/Swift/shell lab tooling (assert-runtime-state, assert-uninstalled-state, CaptureSession.swift, hid-capture-jig-client, etc.) — logic there looked internally consistent.

@malpern

malpern commented Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Post-update hardware control evidence: OTA build f92209f3ea52 verified healthy in valid slot ota_1. A 120-request no-HID soak passed 120/120 with 28.153 ms median, 93.715 ms p95, 105.845 ms max, zero request failures/resets/build changes/server restarts, max firmware handler latency 7.465 ms, and 8.33 MB free heap. Artifact: ~/.local/state/keypath-hid-fixture/control/control-soak-20260729T014347Z.json. Physical button/touch/demo-output validation remains intentionally deferred until an exclusive desktop window.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review scope note: the diff supplied to me was truncated after ~60k bytes (cut off partway through Scripts/lab/hid-capture-jig/...KeycapBurstModel.swift), so I did not see the changes to Sources/KeyPathCLI/.../PrivilegedOperationsRouter.swift, SystemUninstallCommand.swift, Sources/KeyPathCLISupport/CLIReportModels.swift, ServiceBootstrapper.swift, or the associated tests. Those should get a separate look.

Finding — Scripts/build-and-sign.sh: SKIP_DEPLOY=1 doesn't gate the website-publish step

The new SKIP_DEPLOY block wraps the "stop/replace/restart the installed app" section in if [ "${SKIP_DEPLOY:-0}" = "1" ]; then … else … fi, closing the fi right before GHPAGES_DIR=.... The stated intent (per the added comment) is that a matrix build "must not replace or restart the operator's installed app." But the subsequent "Publish help content to website" block is outside that if/else and keeps its original guard ([ -d "$GHPAGES_DIR" ] && [ "${SKIP_WEBSITE:-0}" != "1" ] && [ "${SKIP_NOTARIZE:-}" != "1" ]), so it still runs regardless of SKIP_DEPLOY.

Since matrix/lab builds want "the exact signed artifact" they presumably don't set SKIP_NOTARIZE=1, so on any machine where .worktrees/gh-pages happens to exist (e.g. a release engineer's box also used to produce matrix-test artifacts), running SKIP_DEPLOY=1 ./Scripts/build-and-sign.sh would still publish/copy help content toward the live website — an unintended side effect for a flag meant to make the build a no-op with respect to the operator's environment. Consider also gating the website-publish block on SKIP_DEPLOY, or explicitly documenting that it's intentionally excluded.

No other actionable correctness/security issues found in the reviewed portion (the new assert-runtime-state/assert-uninstalled-state/desktop-bootstrap lab scripts look reasonable; checksums in desktop-bootstrap are valid 64-hex-char SHA-256 values).

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Reviewed the supplied diff (note: it was truncated at 60000 bytes, cutting off before Sources/KeyPathCLI/.../PrivilegedOperationsRouter.swift, SystemUninstallCommand.swift, ServiceBootstrapper.swift, CLIReportModels.swift, Scripts/lab/keypath-lab, Scripts/lab/remote.sh, and most of HIDCaptureJig/main.swift — those are worth a separate pass, especially given AGENTS.md's postcondition-verification requirements for install/uninstall/service-lifecycle code).

One plausible flake in the portion I could review:

Scripts/lab/hid-capture-jig-tool demo subcommand — focus/arm race

"$client" reset >/dev/null
"$client" focus >/dev/null
"$client" arm --run-id offline-demo --expected "$expected" ...

focus and arm are sent as two independent synchronous file-RPC calls with nothing waited on between them. CaptureSession.arm() (Scripts/lab/hid-capture-jig/Sources/HIDCaptureCore/CaptureSession.swift) hard-fails immediately if focused is false at the moment arm is processed:

guard !runID.isEmpty, runID.count <= 80, !expected.isEmpty,
      timeoutMs >= 250, timeoutMs <= 300_000,
      settleMs >= 50, settleMs <= 5000,
      focused
else { ... state = .failed; return false }

If the app's window/app-activation focus notification lands even one runloop tick after the focus RPC returns "ok", the immediately-following arm call will read stale focused=false and the whole demo run fails non-deterministically — independent of anything the operator does. Consider having arm poll/wait briefly for focus (or having the client poll status for focused: true before sending arm) instead of assuming synchronous ordering.

Everything else reviewed (build-and-sign.sh's new SKIP_DEPLOY guard, the new assert-* fixtures, desktop-bootstrap's checksum-verified tool installation) looks sound.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Reviewed the supplied diff (note: it was truncated after ~60KB, so remote.sh, physical-hid-capture-run, scenario-matrix-runner, and most of the Swift HID-capture-jig sources beyond KeycapBurstModel.swift weren't visible to me — worth a follow-up pass on those).

One actionable finding in the portion reviewed:

Scripts/lab/desktop-bootstrap — version/checksum pins are bypassed once a binary already exists. install_desktop_tools() gates each download on [[ ! -x "$local_bin/peekaboo" ]] (same pattern for mcporter, uv), not on the binary's actual version/hash. If a lab host is reused across script revisions (rather than being a freshly provisioned VM every time), a stale binary from a prior bootstrap run will silently stick around even after PEEKABOO_VERSION/PEEKABOO_SHA256 etc. are bumped in this file — the checksum verification in download_verified() never runs against it. Consider checking the installed binary's hash (or a version marker file) against the pinned constant and re-downloading on mismatch, so bumping the pinned version actually takes effect on existing hosts.

Everything else I could see — the SKIP_DEPLOY wrapping in build-and-sign.sh, the new assert-runtime-state/assert-uninstalled-state verifiers, and the hid-capture-jig-client/-tool command protocol — looked structurally sound; no other correctness/security issues stood out.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Correctness bug: CaptureSession.arm() failure isn't sticky, so a bad arm can later self-report as "passed"

Scripts/lab/hid-capture-jig/Sources/HIDCaptureCore/CaptureSession.swift

In arm(), the validation-guard failure path only sets state = .failed — it never sets terminalFailure = true, and deadlineAtNs/settleNs are left at their defaults (deadlineAtNs = nil, settleNs = 250_000_000) because the success-path assignments are skipped:

guard !runID.isEmpty, runID.count <= 80, !expected.isEmpty,
      timeoutMs >= 250, timeoutMs <= 300_000,
      settleMs >= 50, settleMs <= 5000,
      focused
else {
    self.runID = runID
    self.expected = expected
    self.focused = focused
    state = .failed
    return false
}

If runID is non-empty in this failure case (e.g. arm rejected only because focused == false, or timeoutMs/settleMs out of range), subsequent record() calls pass the !runID.isEmpty guard and reach evaluate(). Since terminalFailure is still false, the early-out if terminalFailure { state = .failed; return } never fires, so evaluate() falls through to its normal pass/fail logic. Because deadlineAtNs is nil, the timeout branch never triggers, but the settle branch:

if received == expected, pressedKeyCodes.isEmpty, activeModifiers == 0,
   let lastEventAtNs, nowNs >= lastEventAtNs,
   nowNs - lastEventAtNs >= settleNs
{
    state = .passed
}

can still trigger once received catches up to expected and ~250ms (the default settleNs) elapses with no keys held — silently flipping a rejected/invalid arm from .failed to .passed. This would let a capture run that was armed with invalid parameters report a false PASS to callers polling status/wait in hid-capture-jig-client/hid-capture-jig-tool, undermining the acceptance signal this fixture exists to provide.

Suggested fix: set terminalFailure = true in the guard-else branch (or gate record()/evaluate() on a distinct invalidArm flag) so a failed arm() can never transition to .passed.


Note: this diff is very large (Swift capture-jig core, Python/zsh lab scripts, ESP32/Pico firmware in C, etc.) and was truncated in the review context after ~60KB. I reviewed the visible portion in depth (Swift CaptureSession, the Python assert-runtime-state/assert-uninstalled-state/hid-capture-jig-client scripts, and the desktop-bootstrap/build-and-sign.sh shell changes) and found no other high-confidence issues there. The remainder of the diff (firmware sources, additional test files, remote.sh, scenario-matrix-runner, etc.) was not visible to this review and should get a separate pass.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Reviewed the supplied diff (note: it was truncated after Scripts/lab/hid-capture-jig/Sources/HIDCaptureCore/CaptureSession.swift, so I could not see the actual content of the product-code changes later in the file list — PrivilegedOperationsRouter.swift, SystemUninstallCommand.swift, ServiceBootstrapper.swift, CLIReportModels.swift, and the CLI test updates. Those are the highest-risk changes in this PR since everything else is disposable lab tooling; please make sure they get separate scrutiny, e.g. via ./Scripts/review-gate.sh.).

Within what I could see:

  • Scripts/build-and-sign.sh: the new SKIP_DEPLOY=1 branch wraps the "stop process / replace bundle / restart" block in an if/else/fi, with the closing fi inserted right before the "Publish help content to website" section. The bracing looks correct, but worth double-checking that nothing set inside that now-conditional block (e.g. a deploy path/version variable) is read later in the script when SKIP_DEPLOY=1 skips it — that portion of the file wasn't in the diff, so I couldn't confirm.

  • Scripts/lab/desktop-bootstrap: the old flow auto-clicked "Allow" on the privacy consent sheet; the new flow instead detects the consent dialog via JXA and exits with code 4 (a "checkpoint") for external handling. That's a real behavior change for any caller that expected desktop-bootstrap --install-tools to complete unattended — worth confirming keypath-lab/CI callers were updated to handle exit code 4 (not visible in the supplied diff).

Everything else reviewed (checksum-pinned downloads in desktop-bootstrap, assert-runtime-state, assert-uninstalled-state, CaptureSession.swift state machine) looked internally consistent with no obvious correctness bugs.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Reviewed the supplied diff (truncated at 60000 bytes out of a 144-file/~18.6k-line diff, so most of the new pico-hid-fixture C/ESP32 firmware and the HIDCaptureJig Swift package beyond CaptureSession.swift were not visible to this review).

Scripts/lab/damage-kanata-service — hard failure if the service is already unloaded

set -euo pipefail
...
if [[ -f "$plist" ]]; then
  /usr/bin/sudo -n /bin/launchctl bootout system "$plist"
else
  /usr/bin/sudo -n /bin/launchctl bootout system/com.keypath.kanata
fi

launchctl bootout exits non-zero when the job isn't currently loaded/registered (e.g. it was already booted out by a prior run, or install-runtime hasn't started it yet). Because the script runs under set -e, that failure kills the script immediately — it never reaches the polling loop below that would otherwise treat "no longer registered" as success (print "damage_fixture\tkanata-service-booted-out"). Net effect: this "damage" fixture can spuriously fail scenario runs purely due to fixture-script fragility rather than an actual runtime state problem. Consider tolerating a non-zero bootout exit (e.g. ... || true) and always falling through to the existing poll/verify loop to determine success/failure from actual launchd state.

No other actionable correctness/security issues found in the portion of the diff reviewed (build-and-sign.sh's new SKIP_DEPLOY if/else appears correctly scoped around just the stop/replace/restart block, and the checksum-pinned desktop-bootstrap downloads use full 64-char SHA-256 values as expected).

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