Skip to content

Build BarRobot v3 as a greenfield Pi controller - #3

Open
cmc0619 wants to merge 8 commits into
mainfrom
agent/barrobot-v3-greenfield
Open

cmc0619 wants to merge 8 commits into
mainfrom
agent/barrobot-v3-greenfield

Conversation

@cmc0619

@cmc0619 cmc0619 commented Jul 11, 2026 •

Copy link
Copy Markdown
Owner

Summary

Greenfield BarRobot v3 for the already-purchased Raspberry Pi, DM542T, NEMA-17, 12-slot turret, actuator, PSU, and existing GPIO wiring.

This PR is independent from v2 and targets main directly. It intentionally removes the Flask/Python implementation and does not preserve old routes, configuration files, recipe scaling, or module APIs.

Architecture

  • Node.js 24 LTS + strict TypeScript application
  • Fastify JSON API and dependency-free touchscreen UI
  • versioned atomic JSON persistence
  • explicit CocktailDB synchronization with ambiguous measures kept manual
  • calibrated inventory and dose-error validation
  • immutable drink plans and one FIFO job engine
  • manual-add pauses, cancellation, restart failure semantics, and out-of-band stop
  • native C11 barrobot-motiond process that exclusively owns GPIO through Linux GPIO v2
  • Unix socket boundary between HTTP/application code and motion code

No additional controller board or other hardware is required.

Motion and safety

  • startup is disarmed with actuator low, step low, driver disabled, and position unknown
  • operator-established position is required because the existing hardware has no implemented home sensor
  • exact slot boundaries distribute all 1,600 microsteps around 12 positions
  • shortest-path movement with clockwise tie-break and configurable ramps
  • absolute CLOCK_MONOTONIC deadlines prevent accumulated timer drift
  • optional SCHED_FIFO status is reported honestly
  • stop, interruption, daemon restart, or GPIO failure disarms and invalidates position
  • mutating HTTP endpoints enforce same-origin requests; GET cannot move hardware
  • Node never opens GPIO and the native daemon never accepts network traffic

Deployment

  • hardened barrobot-motion.service and barrobot.service
  • unprivileged barrobot account with only the existing Pi gpio group and motion scheduling capability
  • installer builds release assets under /opt/barrobot
  • application state lives in /var/lib/barrobot/state.json

Validation

  • Prettier format check
  • strict TypeScript production and test compilation
  • typed ESLint rules
  • 21 Node tests passing
  • browser JavaScript syntax check
  • production TypeScript build
  • strict C11 build with -Wall -Wextra -Werror -Wpedantic -Wconversion -Wshadow
  • AddressSanitizer/UndefinedBehaviorSanitizer motion tests
  • exact 1,600-step revolution verification
  • zero production npm vulnerabilities
  • CI includes native ARM64 cross-compilation

See docs/design-v3.md for the complete design and safety contract.

Summary by CodeRabbit

  • New Features

    • Introduced BarRobot v3 with a web-based drink console for machine status, drink selection, inventory, settings, and job progress.
    • Added recipe synchronization, ingredient planning, calibration-aware dispensing, manual-step guidance, and job cancellation/stop controls.
    • Added Raspberry Pi motion control with safe arming, positioning, dispensing, fault handling, and emergency stop behavior.
    • Added installation scripts and system service configuration for production deployment.
  • Documentation

    • Added comprehensive v3 setup, safety, commissioning, architecture, and operating documentation.
  • Tests

    • Added automated coverage for motion control, planning, persistence, HTTP behavior, measurements, and job workflows.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 37 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 64852d06-14b3-4994-8eaf-1ad95e65f3cb

📥 Commits

Reviewing files that changed from the base of the PR and between af4f921 and 7f47f17.

⛔ Files ignored due to path filters (6)
  • docs/screenshots/dashboard-v3.png is excluded by !**/*.png
  • docs/screenshots/inventory-v3.png is excluded by !**/*.png
  • docs/screenshots/job-manual-v3.png is excluded by !**/*.png
  • docs/screenshots/job-v3.png is excluded by !**/*.png
  • docs/screenshots/menu-v3.png is excluded by !**/*.png
  • docs/screenshots/settings-v3.png is excluded by !**/*.png
📒 Files selected for processing (24)
  • README.md
  • deploy/barrobot-motion.service
  • docs/design-v3.md
  • motiond/include/motion.h
  • motiond/src/motion.c
  • motiond/src/server.c
  • motiond/tests/test_motion.c
  • src/adapters/cocktaildb-source.ts
  • src/adapters/motion-socket-client.ts
  • src/adapters/state-validation.ts
  • src/application/barrobot-service.ts
  • src/application/ports.ts
  • src/domain/model.ts
  • src/domain/planner.ts
  • src/domain/seed.ts
  • src/http/server.ts
  • src/main.ts
  • tests/adapters.test.ts
  • tests/helpers.ts
  • tests/planner.test.ts
  • tests/service.test.ts
  • web/app.js
  • web/index.html
  • web/styles.css

Walkthrough

BarRobot v3 replaces the legacy Flask application with a TypeScript HTTP service, C motion daemon, Unix-socket protocol, validated JSON state, job orchestration, static browser UI, systemd deployment, and CI coverage for application and motion builds.

Changes

BarRobot v3 platform

Layer / File(s) Summary
Project contracts and architecture
README.md, docs/design-v3.md, package.json, tsconfig*.json, .prettierrc.json, .prettierignore, .gitignore
Defines the v3 architecture, safety rules, domain contracts, build scripts, formatting rules, and project exclusions.
Motion core and GPIO backend
motiond/include/*, motiond/src/motion.c, motiond/src/gpio_linux.c, motiond/tests/test_motion.c, motiond/Makefile
Implements GPIO-backed motion state, timed movement, dispensing, fault handling, safe outputs, and deterministic C tests.
Motion daemon protocol and lifecycle
motiond/src/server.c
Adds the Unix-socket daemon, command parsing, status responses, client handling, signal shutdown, and realtime scheduling.
Recipe, inventory, and planning behavior
src/domain/*, src/adapters/state-validation.ts, tests/measure.test.ts, tests/planner.test.ts
Adds domain models, measure parsing, inventory validation, seeded recipes, drink planning, menu availability, calibration checks, and related tests.
Persistence and external adapters
src/adapters/*, tests/adapters.test.ts
Adds atomic JSON state persistence, motion-socket communication, protocol parsing, and CocktailDB recipe synchronization.
Application service and HTTP API
src/application/*, src/http/server.ts, src/main.ts, tests/service.test.ts, tests/http.test.ts
Coordinates state and FIFO jobs, manual steps, cancellation and restart recovery, exposes HTTP routes, and wires runtime startup.
Web console and browser behavior
web/*
Adds dashboard, menu, inventory, settings, machine controls, job progress, polling, and responsive styling.
Deployment and validation
deploy/*, scripts/install.sh, .github/workflows/ci.yml, Makefile, tests/helpers.ts
Adds systemd deployment, installation automation, shared build targets, test fakes, and CI jobs for Node, motion, and ARM64 builds.

Sequence Diagram(s)

sequenceDiagram
  participant Browser
  participant HTTPServer
  participant BarRobotService
  participant MotionSocketClient
  participant MotionDaemon

  Browser->>HTTPServer: Submit drink job
  HTTPServer->>BarRobotService: Create and queue plan
  BarRobotService->>MotionSocketClient: Move and dispense
  MotionSocketClient->>MotionDaemon: Unix-socket command
  MotionDaemon-->>MotionSocketClient: Motion response
  MotionSocketClient-->>BarRobotService: Typed result
  BarRobotService-->>HTTPServer: Job status
  HTTPServer-->>Browser: JSON response
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.46% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: rebuilding BarRobot v3 as a greenfield Pi controller.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@cmc0619
cmc0619 marked this pull request as ready for review July 11, 2026 18:53

@coderabbitai coderabbitai 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.

Actionable comments posted: 7

🧹 Nitpick comments (5)
web/app.js (1)

203-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract shared default constants for pressDurationMs and releaseDurationMs.

These values are hardcoded identically in both addInventory (lines 212–213) and saveInventory (lines 243–244). If defaults change, both must be updated in sync.

♻️ Proposed refactor
+const DEFAULT_PRESS_DURATION_MS = 600;
+const DEFAULT_RELEASE_DURATION_MS = 200;
+
 function addInventory(mode) {
   state.inventory.push({
     ...
-    pressDurationMs: 600,
-    releaseDurationMs: 200,
+    pressDurationMs: DEFAULT_PRESS_DURATION_MS,
+    releaseDurationMs: DEFAULT_RELEASE_DURATION_MS,
   });
   ...
 }

 async function saveInventory() {
   ...
   return {
     ...
-    pressDurationMs: 600,
-    releaseDurationMs: 200,
+    pressDurationMs: DEFAULT_PRESS_DURATION_MS,
+    releaseDurationMs: DEFAULT_RELEASE_DURATION_MS,
   };
   ...
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/app.js` around lines 203 - 247, Extract shared constants for the default
press and release durations, then replace the duplicated 600 and 200 literals in
both addInventory and saveInventory with those constants. Keep the existing
values and behavior unchanged.
src/adapters/cocktaildb-source.ts (1)

36-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider Promise.allSettled for resilient free-tier sync.

fetchFree fires 26 parallel requests to the CocktailDB free API. A single rate-limited response (HTTP 429) or transient failure causes Promise.all to reject, failing the entire sync. Using Promise.allSettled and filtering fulfilled results would allow partial sync completion, letting the operator retry only the failed letters.

♻️ Proposed refactor for resilient free-tier fetching
   private async fetchFree(): Promise<CocktailDbDrink[]> {
-    const batches = await Promise.all(
+    const results = await Promise.allSettled(
       Array.from("abcdefghijklmnopqrstuvwxyz").map(async (letter) =>
         fetchJson(`https://www.thecocktaildb.com/api/json/v1/1/search.php?f=${letter}`),
       ),
     );
-    return batches.flatMap((batch) => batch.drinks ?? []);
+    return results
+      .filter((result): result is PromiseFulfilledResult<CocktailDbResponse> =>
+        result.status === "fulfilled")
+      .flatMap((result) => result.value.drinks ?? []);
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/adapters/cocktaildb-source.ts` around lines 36 - 43, Update fetchFree to
use Promise.allSettled for the per-letter CocktailDB requests, then retain only
fulfilled results before flattening their drinks arrays. Preserve the existing
letter iteration and empty-drinks fallback while allowing individual rate-limit
or transient failures to be skipped.
motiond/src/server.c (1)

121-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Call motion_destroy and gpio_linux_destroy on shutdown.

The (void)motion; (void)gpio; suppressions indicate these are intentionally not cleaned up. While motion_stop (called by the signal thread) sets safe outputs and the kernel releases GPIO on process exit, gpio_linux_destroy is documented to "restore safe output values and release the GPIO line request" and motion_destroy destroys the mutex. Explicit cleanup ensures proper resource release and safe GPIO teardown.

♻️ Proposed fix
     pthread_join(signal_thread, NULL);
     unlink(config.socket_path);
-    (void)motion;
-    (void)gpio;
+    motion_destroy(motion);
+    gpio_linux_destroy(gpio);
     return EXIT_SUCCESS;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@motiond/src/server.c` around lines 121 - 125, Replace the unused-variable
suppressions in the shutdown path after pthread_join with calls to
motion_destroy for motion and gpio_linux_destroy for gpio, before unlinking the
socket and returning. Preserve the existing shutdown ordering so the signal
thread has finished before destroying these resources.
motiond/tests/test_motion.c (1)

63-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding tests for disarm, reset, invalid slot, and BUSY rejection.

The current tests cover startup, arm, full revolution, tie-breaking, dispense, and stop — the most critical safety paths. Missing cases include: motion_disarm output behavior, motion_reset fault recovery, invalid slot rejection (MOTION_INVALID), and concurrent command rejection (MOTION_BUSY). Adding these would strengthen confidence in the state machine's edge cases.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@motiond/tests/test_motion.c` around lines 63 - 128, Add focused tests
alongside the existing motion tests for the missing state-machine cases: verify
motion_disarm’s return value and GPIO output behavior, confirm motion_reset
recovers from a latched fault, assert invalid slot commands return
MOTION_INVALID without changing state, and exercise an in-progress command to
ensure concurrent commands return MOTION_BUSY. Register each new test in main,
reusing the existing fake_gpio and motion lifecycle patterns.
src/adapters/json-state-repository.ts (1)

38-56: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Temp file leak on write failure.

If file.sync() or writeFile throws, the finally block closes the handle but never unlinks temporary, leaving orphaned .tmp files in the state directory. Over time a flaky disk could accumulate stale temp files. Consider cleaning up in a catch/finally.

♻️ Optional cleanup on failure
   private async writeAtomic(state: StateDocument): Promise<void> {
     const directory = dirname(this.path);
     const temporary = `${this.path}.${process.pid}.${Date.now()}.tmp`;
     await mkdir(directory, { recursive: true });
     const file = await open(temporary, "wx", 0o600);
     try {
       await file.writeFile(`${JSON.stringify(state, null, 2)}\n`, "utf8");
       await file.sync();
     } catch (writeError) {
       await file.close().catch(() => undefined);
       await unlink(temporary).catch(() => undefined);
       throw writeError;
     }
     await file.close();
     await rename(temporary, this.path);
     const directoryHandle = await open(directory, "r");
     try {
       await directoryHandle.sync();
     } finally {
       await directoryHandle.close();
     }
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/adapters/json-state-repository.ts` around lines 38 - 56, Update
writeAtomic to remove the temporary file when writeFile or file.sync fails,
while still closing the file handle and preserving the existing rename path on
success. Use the existing temporary variable and ensure cleanup errors do not
replace the original write failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@deploy/barrobot.service`:
- Line 16: Update the BARROBOT_HOST setting in the service configuration to bind
the actuator API only to loopback, preserving local touchscreen access without
exposing unauthenticated control endpoints on every network interface.

In `@motiond/src/server.c`:
- Around line 98-121: Ensure the accept loop’s non-EINTR error path also
terminates the signal thread before joining it: use pthread_kill with the
already-blocked SIGTERM signal to wake sigwait, then proceed to
pthread_join(signal_thread, NULL). Preserve the existing EINTR retry and normal
shutdown behavior.

In `@scripts/install.sh`:
- Around line 26-28: Update the service activation commands in
scripts/install.sh to separate enabling from restarting for
barrobot-motion.service and barrobot.service. Replace the --now behavior with
explicit systemctl enable followed by systemctl restart so newly installed
binaries are activated on both initial installation and upgrades.

In `@src/application/barrobot-service.ts`:
- Around line 221-270: Move the initial persist call in runJob into the try
block so activeJobId is always cleared by finally when persistence fails. Ensure
the failure-handling persist in runJob’s catch cannot throw an unhandled
rejection, and add rejection handling to pump so asynchronous job-processing
failures are contained. Preserve the existing job failure status updates and
cleanup behavior.

In `@src/http/server.ts`:
- Around line 39-61: Update the catch fallback in the /api/status handler to
derive queuedJobs and activeJob from the existing state snapshot, matching the
successful return branch. Preserve the offline motion response, but do not
replace snapshot-derived job information with zero or null when service.status()
fails.

In `@web/app.js`:
- Around line 169-184: Update the data-mode attribute interpolation in
renderInventory to pass item.mode through escapeAttribute, matching the escaping
used for other user-controlled attribute values while preserving the existing
mode selection behavior.

In `@web/index.html`:
- Around line 135-149: The deployed UI must not present motionSocket,
listenHost, and listenPort as immediately effective settings. Remove or disable
these inputs in web/index.html, or clearly mark them as requiring a systemd
service-unit change and service restart; preserve the maxDoseErrorPercent field.

---

Nitpick comments:
In `@motiond/src/server.c`:
- Around line 121-125: Replace the unused-variable suppressions in the shutdown
path after pthread_join with calls to motion_destroy for motion and
gpio_linux_destroy for gpio, before unlinking the socket and returning. Preserve
the existing shutdown ordering so the signal thread has finished before
destroying these resources.

In `@motiond/tests/test_motion.c`:
- Around line 63-128: Add focused tests alongside the existing motion tests for
the missing state-machine cases: verify motion_disarm’s return value and GPIO
output behavior, confirm motion_reset recovers from a latched fault, assert
invalid slot commands return MOTION_INVALID without changing state, and exercise
an in-progress command to ensure concurrent commands return MOTION_BUSY.
Register each new test in main, reusing the existing fake_gpio and motion
lifecycle patterns.

In `@src/adapters/cocktaildb-source.ts`:
- Around line 36-43: Update fetchFree to use Promise.allSettled for the
per-letter CocktailDB requests, then retain only fulfilled results before
flattening their drinks arrays. Preserve the existing letter iteration and
empty-drinks fallback while allowing individual rate-limit or transient failures
to be skipped.

In `@src/adapters/json-state-repository.ts`:
- Around line 38-56: Update writeAtomic to remove the temporary file when
writeFile or file.sync fails, while still closing the file handle and preserving
the existing rename path on success. Use the existing temporary variable and
ensure cleanup errors do not replace the original write failure.

In `@web/app.js`:
- Around line 203-247: Extract shared constants for the default press and
release durations, then replace the duplicated 600 and 200 literals in both
addInventory and saveInventory with those constants. Keep the existing values
and behavior unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b179f12e-80c4-4bab-8888-939cd34029f9

📥 Commits

Reviewing files that changed from the base of the PR and between affb9ad and af4f921.

⛔ Files ignored due to path filters (3)
  • classes.dot is excluded by !**/*.dot
  • package-lock.json is excluded by !**/package-lock.json
  • static/placeholder.png is excluded by !**/*.png
📒 Files selected for processing (57)
  • .github/workflows/ci.yml
  • .gitignore
  • .prettierignore
  • .prettierrc.json
  • Makefile
  • README.md
  • app.py
  • bottle_config.json
  • deploy/barrobot-motion.service
  • deploy/barrobot.service
  • docs/design-v3.md
  • eslint.config.js
  • hardware.py
  • motiond/Makefile
  • motiond/include/gpio_linux.h
  • motiond/include/motion.h
  • motiond/src/gpio_linux.c
  • motiond/src/motion.c
  • motiond/src/server.c
  • motiond/tests/test_motion.c
  • package.json
  • scripts/install.sh
  • src/adapters/cocktaildb-source.ts
  • src/adapters/json-state-repository.ts
  • src/adapters/motion-socket-client.ts
  • src/adapters/state-validation.ts
  • src/application/barrobot-service.ts
  • src/application/ports.ts
  • src/domain/errors.ts
  • src/domain/measure.ts
  • src/domain/model.ts
  • src/domain/normalize.ts
  • src/domain/planner.ts
  • src/domain/seed.ts
  • src/http/server.ts
  • src/main.ts
  • static/style.css
  • systemd-barrobot.service
  • templates/base.html
  • templates/configure.html
  • templates/drink.html
  • templates/menu.html
  • templates/motor_controls.html
  • templates/suggestions.html
  • templates/suggestions2.html
  • tests/adapters.test.ts
  • tests/helpers.ts
  • tests/http.test.ts
  • tests/measure.test.ts
  • tests/planner.test.ts
  • tests/service.test.ts
  • tsconfig.json
  • tsconfig.test.json
  • version.txt
  • web/app.js
  • web/index.html
  • web/styles.css
💤 Files with no reviewable changes (13)
  • hardware.py
  • templates/configure.html
  • templates/suggestions.html
  • static/style.css
  • templates/suggestions2.html
  • templates/menu.html
  • templates/base.html
  • bottle_config.json
  • templates/drink.html
  • systemd-barrobot.service
  • version.txt
  • templates/motor_controls.html
  • app.py

Comment thread deploy/barrobot.service
Environment=NODE_ENV=production
Environment=BARROBOT_STATE_PATH=/var/lib/barrobot/state.json
Environment=BARROBOT_MOTION_SOCKET=/run/barrobot/motion.sock
Environment=BARROBOT_HOST=0.0.0.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not expose the actuator API unauthenticated.

Line 16 binds the service to every interface, while the API permits arm, move, dispense-job, and stop actions without authentication. The origin hook is not network access control. Bind to loopback for the local touchscreen, or put remote access behind authenticated TLS and firewall rules.

Proposed safe default
-Environment=BARROBOT_HOST=0.0.0.0
+Environment=BARROBOT_HOST=127.0.0.1
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Environment=BARROBOT_HOST=0.0.0.0
Environment=BARROBOT_HOST=127.0.0.1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deploy/barrobot.service` at line 16, Update the BARROBOT_HOST setting in the
service configuration to bind the actuator API only to loopback, preserving
local touchscreen access without exposing unauthenticated control endpoints on
every network interface.

Comment thread motiond/src/server.c
Comment on lines +98 to +121
for (;;) {
const int client_fd = accept4(server_fd, NULL, NULL, SOCK_CLOEXEC);
if (client_fd < 0) {
if (errno == EINTR) {
continue;
}
break;
}
struct client_context *context = malloc(sizeof(*context));
if (context == NULL) {
close(client_fd);
continue;
}
context->client_fd = client_fd;
context->motion = motion;
pthread_t thread;
if (pthread_create(&thread, NULL, handle_client, context) != 0) {
close(client_fd);
free(context);
continue;
}
pthread_detach(thread);
}
pthread_join(signal_thread, NULL);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Potential deadlock if accept4 fails for a non-signal reason.

If accept4 returns an error other than EINTR that is not caused by the signal thread's shutdown (e.g., EMFILE from too many open file descriptors), the main thread breaks out of the loop and calls pthread_join(signal_thread, NULL). Since the signal thread is still blocked in sigwait, this join blocks indefinitely. Consider sending a signal to the signal thread or using pthread_cancel before the join.

🛡️ Proposed fix
     }
-    pthread_join(signal_thread, NULL);
+    pthread_kill(signal_thread, SIGTERM);
+    pthread_join(signal_thread, NULL);
     unlink(config.socket_path);

Since SIGTERM is already in the blocked signal set and sigwait will catch it, pthread_kill unblocks the signal thread cleanly.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (;;) {
const int client_fd = accept4(server_fd, NULL, NULL, SOCK_CLOEXEC);
if (client_fd < 0) {
if (errno == EINTR) {
continue;
}
break;
}
struct client_context *context = malloc(sizeof(*context));
if (context == NULL) {
close(client_fd);
continue;
}
context->client_fd = client_fd;
context->motion = motion;
pthread_t thread;
if (pthread_create(&thread, NULL, handle_client, context) != 0) {
close(client_fd);
free(context);
continue;
}
pthread_detach(thread);
}
pthread_join(signal_thread, NULL);
for (;;) {
const int client_fd = accept4(server_fd, NULL, NULL, SOCK_CLOEXEC);
if (client_fd < 0) {
if (errno == EINTR) {
continue;
}
break;
}
struct client_context *context = malloc(sizeof(*context));
if (context == NULL) {
close(client_fd);
continue;
}
context->client_fd = client_fd;
context->motion = motion;
pthread_t thread;
if (pthread_create(&thread, NULL, handle_client, context) != 0) {
close(client_fd);
free(context);
continue;
}
pthread_detach(thread);
}
pthread_kill(signal_thread, SIGTERM);
pthread_join(signal_thread, NULL);
unlink(config.socket_path);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@motiond/src/server.c` around lines 98 - 121, Ensure the accept loop’s
non-EINTR error path also terminates the signal thread before joining it: use
pthread_kill with the already-blocked SIGTERM signal to wake sigwait, then
proceed to pthread_join(signal_thread, NULL). Preserve the existing EINTR retry
and normal shutdown behavior.

Comment thread scripts/install.sh
Comment on lines +26 to +28
systemctl daemon-reload
systemctl enable --now barrobot-motion.service barrobot.service
systemctl --no-pager --full status barrobot-motion.service barrobot.service

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Use restart instead of --now to pick up new binaries on upgrade.

systemctl enable --now starts services on first install but does not restart already-running services. On a re-run (upgrade), the new binary is on disk while the old process keeps running. Separating enable from restart ensures the new binary is activated on both first install and upgrade.

🔧 Proposed fix
 systemctl daemon-reload
-systemctl enable --now barrobot-motion.service barrobot.service
+systemctl enable barrobot-motion.service barrobot.service
+systemctl restart barrobot-motion.service barrobot.service
 systemctl --no-pager --full status barrobot-motion.service barrobot.service
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
systemctl daemon-reload
systemctl enable --now barrobot-motion.service barrobot.service
systemctl --no-pager --full status barrobot-motion.service barrobot.service
systemctl daemon-reload
systemctl enable barrobot-motion.service barrobot.service
systemctl restart barrobot-motion.service barrobot.service
systemctl --no-pager --full status barrobot-motion.service barrobot.service
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install.sh` around lines 26 - 28, Update the service activation
commands in scripts/install.sh to separate enabling from restarting for
barrobot-motion.service and barrobot.service. Replace the --now behavior with
explicit systemctl enable followed by systemctl restart so newly installed
binaries are activated on both initial installation and upgrades.

Comment thread src/application/barrobot-service.ts Resolved
Comment thread src/http/server.ts
Comment on lines +39 to +61
server.get("/api/status", async () => {
const state = service.snapshot();
try {
return {
motion: await service.status(),
queuedJobs: state.jobs.filter((job) => job.status === "queued").length,
activeJob:
state.jobs.find((job) => job.status === "running" || job.status === "waiting_manual") ??
null,
};
} catch (error) {
return {
motion: {
state: "offline",
armed: false,
position: null,
realtime: false,
error: error instanceof Error ? error.message : "Motion service offline",
},
queuedJobs: 0,
activeJob: null,
};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve job state when motion status is unavailable.

If service.status() fails, the fallback reports no queued or active job even when the snapshot already contains one. A temporary daemon outage can therefore hide a waiting-manual drink and its cancel/continue controls. Return the snapshot-derived queue and active job in both branches.

Proposed fix
   server.get("/api/status", async () => {
     const state = service.snapshot();
+    const queuedJobs = state.jobs.filter((job) => job.status === "queued").length;
+    const activeJob =
+      state.jobs.find((job) => job.status === "running" || job.status === "waiting_manual") ??
+      null;
     try {
       return {
         motion: await service.status(),
-        queuedJobs: state.jobs.filter((job) => job.status === "queued").length,
-        activeJob:
-          state.jobs.find((job) => job.status === "running" || job.status === "waiting_manual") ??
-          null,
+        queuedJobs,
+        activeJob,
       };
     } catch (error) {
       return {
         motion: {
           state: "offline",
           armed: false,
           position: null,
           realtime: false,
           error: error instanceof Error ? error.message : "Motion service offline",
         },
-        queuedJobs: 0,
-        activeJob: null,
+        queuedJobs,
+        activeJob,
       };
     }
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
server.get("/api/status", async () => {
const state = service.snapshot();
try {
return {
motion: await service.status(),
queuedJobs: state.jobs.filter((job) => job.status === "queued").length,
activeJob:
state.jobs.find((job) => job.status === "running" || job.status === "waiting_manual") ??
null,
};
} catch (error) {
return {
motion: {
state: "offline",
armed: false,
position: null,
realtime: false,
error: error instanceof Error ? error.message : "Motion service offline",
},
queuedJobs: 0,
activeJob: null,
};
}
server.get("/api/status", async () => {
const state = service.snapshot();
const queuedJobs = state.jobs.filter((job) => job.status === "queued").length;
const activeJob =
state.jobs.find((job) => job.status === "running" || job.status === "waiting_manual") ??
null;
try {
return {
motion: await service.status(),
queuedJobs,
activeJob,
};
} catch (error) {
return {
motion: {
state: "offline",
armed: false,
position: null,
realtime: false,
error: error instanceof Error ? error.message : "Motion service offline",
},
queuedJobs,
activeJob,
};
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/http/server.ts` around lines 39 - 61, Update the catch fallback in the
/api/status handler to derive queuedJobs and activeJob from the existing state
snapshot, matching the successful return branch. Preserve the offline motion
response, but do not replace snapshot-derived job information with zero or null
when service.status() fails.

Comment thread web/app.js
Comment on lines +169 to +184
function renderInventory() {
elements["inventory-list"].innerHTML = state.inventory
.map(
(item) => `
<article class="inventory-row" data-id="${escapeAttribute(item.id)}" data-mode="${item.mode}">
<label>Name<input data-field="name" value="${escapeAttribute(item.name)}" /></label>
<label>Aliases<input data-field="aliases" value="${escapeAttribute(item.aliases.join(", "))}" /></label>
<label>Mode<select data-field="mode">
<option value="bottle" ${item.mode === "bottle" ? "selected" : ""}>Bottle</option>
<option value="pantry" ${item.mode === "pantry" ? "selected" : ""}>Pantry</option>
</select></label>
<label>Slot<input data-field="slot" type="number" min="1" max="12" value="${item.slot === null ? "" : item.slot + 1}" ${item.mode === "pantry" ? "disabled" : ""} /></label>
<label>ml / press<input data-field="mlPerPress" type="number" min="0.1" step="0.1" value="${item.mlPerPress ?? ""}" ${item.mode === "pantry" ? "disabled" : ""} /></label>
<button class="button danger" data-remove="${escapeAttribute(item.id)}">Remove</button>
</article>`,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Escape item.mode in the data-mode attribute.

Line 173 interpolates item.mode directly into an HTML attribute without escapeAttribute, unlike all other user-controlled values in this template. While the server validates mode to "bottle" or "pantry", defense-in-depth requires escaping it consistently.

🔒 Proposed fix
-      <article class="inventory-row" data-id="${escapeAttribute(item.id)}" data-mode="${item.mode}">
+      <article class="inventory-row" data-id="${escapeAttribute(item.id)}" data-mode="${escapeAttribute(item.mode)}">
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function renderInventory() {
elements["inventory-list"].innerHTML = state.inventory
.map(
(item) => `
<article class="inventory-row" data-id="${escapeAttribute(item.id)}" data-mode="${item.mode}">
<label>Name<input data-field="name" value="${escapeAttribute(item.name)}" /></label>
<label>Aliases<input data-field="aliases" value="${escapeAttribute(item.aliases.join(", "))}" /></label>
<label>Mode<select data-field="mode">
<option value="bottle" ${item.mode === "bottle" ? "selected" : ""}>Bottle</option>
<option value="pantry" ${item.mode === "pantry" ? "selected" : ""}>Pantry</option>
</select></label>
<label>Slot<input data-field="slot" type="number" min="1" max="12" value="${item.slot === null ? "" : item.slot + 1}" ${item.mode === "pantry" ? "disabled" : ""} /></label>
<label>ml / press<input data-field="mlPerPress" type="number" min="0.1" step="0.1" value="${item.mlPerPress ?? ""}" ${item.mode === "pantry" ? "disabled" : ""} /></label>
<button class="button danger" data-remove="${escapeAttribute(item.id)}">Remove</button>
</article>`,
)
function renderInventory() {
elements["inventory-list"].innerHTML = state.inventory
.map(
(item) => `
<article class="inventory-row" data-id="${escapeAttribute(item.id)}" data-mode="${escapeAttribute(item.mode)}">
<label>Name<input data-field="name" value="${escapeAttribute(item.name)}" /></label>
<label>Aliases<input data-field="aliases" value="${escapeAttribute(item.aliases.join(", "))}" /></label>
<label>Mode<select data-field="mode">
<option value="bottle" ${item.mode === "bottle" ? "selected" : ""}>Bottle</option>
<option value="pantry" ${item.mode === "pantry" ? "selected" : ""}>Pantry</option>
</select></label>
<label>Slot<input data-field="slot" type="number" min="1" max="12" value="${item.slot === null ? "" : item.slot + 1}" ${item.mode === "pantry" ? "disabled" : ""} /></label>
<label>ml / press<input data-field="mlPerPress" type="number" min="0.1" step="0.1" value="${item.mlPerPress ?? ""}" ${item.mode === "pantry" ? "disabled" : ""} /></label>
<button class="button danger" data-remove="${escapeAttribute(item.id)}">Remove</button>
</article>`,
)
🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 169-184: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: elements["inventory-list"].innerHTML = state.inventory
.map(
(item) => <article class="inventory-row" data-id="${escapeAttribute(item.id)}" data-mode="${item.mode}"> <label>Name<input data-field="name" value="${escapeAttribute(item.name)}" /></label> <label>Aliases<input data-field="aliases" value="${escapeAttribute(item.aliases.join(", "))}" /></label> <label>Mode<select data-field="mode"> <option value="bottle" ${item.mode === "bottle" ? "selected" : ""}>Bottle</option> <option value="pantry" ${item.mode === "pantry" ? "selected" : ""}>Pantry</option> </select></label> <label>Slot<input data-field="slot" type="number" min="1" max="12" value="${item.slot === null ? "" : item.slot + 1}" ${item.mode === "pantry" ? "disabled" : ""} /></label> <label>ml / press<input data-field="mlPerPress" type="number" min="0.1" step="0.1" value="${item.mlPerPress ?? ""}" ${item.mode === "pantry" ? "disabled" : ""} /></label> <button class="button danger" data-remove="${escapeAttribute(item.id)}">Remove</button> </article>,
)
.join("")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').

(inner-outer-html)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/app.js` around lines 169 - 184, Update the data-mode attribute
interpolation in renderInventory to pass item.mode through escapeAttribute,
matching the escaping used for other user-controlled attribute values while
preserving the existing mode selection behavior.

Comment thread web/index.html
Comment on lines +135 to +149
<label>
Motion socket
<input name="motionSocket" />
</label>
<label>
Maximum dose error (%)
<input name="maxDoseErrorPercent" type="number" min="0" max="100" step="1" />
</label>
<label>
Listen host
<input name="listenHost" />
</label>
<label>
Listen port
<input name="listenPort" type="number" min="1" max="65535" />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not present deployment-overridden settings as effective.

These fields save successfully, but production always overrides motionSocket, listenHost, and listenPort through systemd environment variables. The socket value is therefore never used, and host/port are also only read at startup. Remove or disable these fields in the deployed UI, or explicitly show that they require a service-unit change/restart.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/index.html` around lines 135 - 149, The deployed UI must not present
motionSocket, listenHost, and listenPort as immediately effective settings.
Remove or disable these inputs in web/index.html, or clearly mark them as
requiring a systemd service-unit change and service restart; preserve the
maxDoseErrorPercent field.

Capture the four console tabs plus the running and manual-step job states
against a demo 12-bottle cocktail profile, and document each in a new
'The UI' section.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

BarRobot v3 replaces the legacy Flask controller with a TypeScript application and isolated native motion daemon.

  • Adds calibrated recipe planning, persistent inventory profiles, and a FIFO drink-job engine.
  • Adds a Fastify API and dependency-free touchscreen interface.
  • Adds Linux GPIO v2 motion control, Unix-socket communication, deployment units, installation tooling, and CI coverage.

Confidence Score: 3/5

This PR should not merge until concurrent stop and disarm requests are guaranteed to prevent the dispense loop from re-energizing the actuator.

The independently handled motion commands leave a check-to-assert race in the core safety path, allowing the actuator to turn back on after STOP or DISARM has reported success.

Files Needing Attention: motiond/src/motion.c and motiond/src/server.c

Important Files Changed

Filename Overview
motiond/src/motion.c Implements motion safety and dispensing, but stop/disarm can race with actuator assertion during an active dispense.
motiond/src/server.c Implements the concurrent Unix-socket command server and exposes the stop/dispense concurrency underlying the motion race.
src/application/barrobot-service.ts Adds persisted FIFO job orchestration, cancellation, manual pauses, and restart failure handling.
src/http/server.ts Adds the JSON API, same-origin mutation checks, static UI serving, and application-service routing.
src/domain/planner.ts Adds calibrated immutable drink planning with automatic and manual ingredient steps.
motiond/src/gpio_linux.c Adds exclusive Linux GPIO v2 line ownership and raw line operations for the motion controller.
scripts/install.sh Builds and installs the application, native daemon, state directory, service account, and systemd units.

Sequence Diagram

sequenceDiagram
  participant App as TypeScript application
  participant Server as Motion socket server
  participant Motion as Motion controller
  participant GPIO as Linux GPIO v2
  App->>Server: DISPENSE
  Server->>Motion: motion_dispense()
  Motion->>GPIO: actuator and motion outputs
  App->>Server: STOP (separate connection)
  Server->>Motion: motion_stop()
  Motion->>GPIO: safe outputs
Loading

Fix All in Claude Code Fix All in Codex

Reviews (1): Last reviewed commit: "Add v3 UI screenshots to README (#6)" | Re-trigger Greptile

Comment thread motiond/src/motion.c
Comment on lines +354 to +358
if (atomic_load(&instance->stop_requested) || !atomic_load(&instance->armed)) {
result = fail_motion(instance, MOTION_STOPPED);
goto done;
}
if (set_logical_line(instance, MOTION_LINE_ACTUATOR, true) != 0 ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Stop race reasserts actuator

When STOP or DISARM runs after the dispense loop checks stop_requested and armed but before it asserts the actuator, the safety command clears the outputs and returns success before the dispense thread energizes the actuator again, causing it to remain active for up to the configured 60-second press duration.

Fix in Claude Code Fix in Codex

@codacy-production

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 12 critical · 17 high · 1 medium

Alerts:
⚠ 30 issues (≤ 0 issues of at least minor severity)

Results:
30 new issues

Category Results
ErrorProne 1 medium
6 high
Security 12 critical
11 high

View in Codacy

🟢 Metrics 717 complexity · 25 duplication

Metric Results
Complexity 717
Duplication 25

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production 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.

Pull Request Overview

While the architectural transition to Node.js and a native C11 daemon is sound and covers all primary acceptance criteria, the PR is currently not up to standards. A critical security risk was identified in the motion daemon's socket handling where unsafe string copying could lead to buffer overflows or non-terminated strings.

Furthermore, the implementation of external API fetching is likely to trigger rate limits, and there is a misalignment between the documented safety slowdown percentages and the actual multipliers used in the service logic. Core modules like motion.c and state-validation.ts exhibit very high complexity, which may impact future maintainability despite the presence of automated tests.

About this PR

  • The PR has introduced 30 new quality issues and is currently flagged as not up to standards. Please review the specific findings in the file comments to address these regressions.

Test suggestions

  • Found recommended test scenario: Verify exact 1,600 step revolution counts via fake GPIO backend
  • Found recommended test scenario: Verify S-curve ramp starts without initial velocity jumps
  • Found recommended test scenario: Verify shortest path logic with clockwise tie-break on 6-slot moves
  • Found recommended test scenario: Verify arming is rejected until position is established
  • Found recommended test scenario: Verify stop command latches fault and invalidates turret position
  • Found recommended test scenario: Verify measure parsing for volume (ml) vs. manual terms (e.g., 'wedge')
  • Found recommended test scenario: Verify recipe rejection when dose error exceeds configured tolerance
  • Found recommended test scenario: Verify FIFO job execution correctly pauses for manual acknowledgement
  • Found recommended test scenario: Verify cross-origin POST requests are rejected at the HTTP boundary
  • Found recommended test scenario: Verify atomic state replacement and version validation during persistence
Low confidence findings
  • Several core files, including motiond/src/motion.c (complexity: 145) and src/adapters/state-validation.ts (complexity: 132), exhibit high cyclomatic complexity. While these implement critical logic, consider refactoring them into smaller, more testable units in a follow-up PR.

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

Comment thread motiond/src/server.c
struct sockaddr_un address;
memset(&address, 0, sizeof(address));
address.sun_family = AF_UNIX;
strncpy(address.sun_path, path, sizeof(address.sun_path) - 1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 HIGH RISK

The strncpy function is easily used incorrectly and can leave strings without null-termination. Consider using snprintf for safer string copying.

Suggested change
strncpy(address.sun_path, path, sizeof(address.sun_path) - 1);
snprintf(address.sun_path, sizeof(address.sun_path), "%s", path);


private async fetchFree(): Promise<CocktailDbDrink[]> {
const batches = await Promise.all(
Array.from("abcdefghijklmnopqrstuvwxyz").map(async (letter) =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MEDIUM RISK

Suggestion: The application triggers up to 26 concurrent HTTP requests to CocktailDB. This is likely to be rate-limited by the service.

Try running the following prompt in your coding agent:

In src/adapters/cocktaildb-source.ts, refactor fetchFree to fetch the batches sequentially using a loop instead of Promise.all to respect external API limits.

Comment thread src/domain/measure.ts
return null;
}
const multiplier = UNIT_TO_ML[unit[1]];
return multiplier === undefined ? null : Math.round(quantity * multiplier * 100) / 100;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MEDIUM RISK

Nitpick: The 'multiplier' is guaranteed to be defined because the preceding regex match only allows units that exist in the UNIT_TO_ML map. The undefined check can be removed for cleaner logic.

Suggested change
return multiplier === undefined ? null : Math.round(quantity * multiplier * 100) / 100;
return Math.round(quantity * multiplier * 100) / 100;

}
for (let index = job.currentStep; index < job.plan.steps.length; index += 1) {
const step = job.plan.steps[index];
if (!step) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MEDIUM RISK

Nitpick: This condition is unnecessary because 'step' is guaranteed to be defined for every iteration within the array bounds.

Try running the following prompt in your IDE agent:

Remove the redundant if (!step) check and the associated error throwing from src/application/barrobot-service.ts around line 277, as the loop bounds guarantee the element exists.

reject(error);
};
socket.setEncoding("utf8");
socket.setTimeout(timeoutMs, () => fail(new MotionError("TIMEOUT", `${command} timed out`)));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ LOW RISK

Suggestion: Avoid using a void expression in a shorthand return. Braces should be used to signal that the function is for side effects only.

Suggested change
socket.setTimeout(timeoutMs, () => fail(new MotionError("TIMEOUT", `${command} timed out`)));
socket.setTimeout(timeoutMs, () => { fail(new MotionError("TIMEOUT", `${command} timed out`)); });

{ x: 0, y: 0 },
);
const imbalance = Math.hypot(vector.x, vector.y) / total;
return imbalance >= 0.65 ? 150 : imbalance >= 0.4 ? 125 : 100;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ LOW RISK

Suggestion: The timing multipliers (125, 150) result in a 20% and 33% speed reduction respectively (1/1.25 and 1/1.5). To achieve the 50% slowdown mentioned in the description, a timing multiplier of 200 would be required.

Comment thread motiond/src/server.c
}
pthread_join(signal_thread, NULL);
unlink(config.socket_path);
(void)motion;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ LOW RISK

Nitpick: The motion and gpio objects are not explicitly destroyed on exit. Calling the destroyers ensures clean closure of file descriptors and state.

Try running the following prompt in your coding agent:

In motiond/src/server.c, replace the (void) casts at the end of main with calls to motion_destroy(motion) and gpio_linux_destroy(gpio) after the signal thread has joined.

cursor Bot pushed a commit that referenced this pull request Sep 7, 2026
Codacy gates pull requests on new issues, and it decides what is new by
path. Moving v1 into v1/ and bringing v2/ and v3/ onto main therefore
presents both rewrites as brand-new code, so this PR inherits the whole
backlog of the two open rewrite PRs at once: 62 issues from #2 plus 30
from #3, against a limit of zero.

Four of those were real and are fixed in the preceding commit. The rest
are analysers reaching wrong conclusions because they run with their own
defaults rather than the configuration each version is built with:

  - Bandit reports B101 on every pytest assert and reads conftest.py's
    throwaway Flask signing key as a hardcoded password.
  - Codacy's ESLint has no access to v3's tsconfig, so it cannot see
    noUncheckedIndexedAccess and reports the undefined-guards that make
    v3 compile as conditions that are always false.
  - Opengrep flags a state repository whose path is configuration, and
    two innerHTML writes whose inputs are already escaped through the
    DOM's own textContent round-trip.
  - flawfinder flags every strncpy in motiond, all of which write
    memset-zeroed fixed buffers under an explicit sizeof - 1 bound.

Each exclusion names one analyser and one path and carries its reason.
No analyser is turned off repository-wide: v2's application code, 986
lines across eight files, stays under Bandit, and v3 keeps linting src/
in CI with typescript-eslint against the real tsconfig.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request Sep 7, 2026
Codacy's dependency scanner reported these against package-lock.json.
They are new since PR #3 was last analysed, not something the
restructure introduced:

  @fastify/static 10.1.0  path traversal past route guards, and
                          authorization bypass via non-canonical paths
  fastify 5.10.0          request spoofing through numeric trustProxy,
                          and schema validation bypass on root primitives
  fast-uri 3.1.3, 4.1.0   six findings, mostly SSRF and host confusion
                          from parsing inconsistencies
  find-my-way 9.6.0       HTTP/2 denial of service
  brace-expansion 5.0.7   denial of service through memory exhaustion

Every fix was already inside the semver ranges package.json declares, so
this is a lockfile refresh with no manifest change: fastify 5.12.3,
@fastify/static 10.1.3, fast-uri 3.1.7 and 4.1.4, find-my-way 9.9.0,
brace-expansion 5.0.9. npm audit is clean, npm ci reproduces the tree,
and prettier, tsc, eslint, the build, and all 25 tests pass on it.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>
cmc0619 added a commit that referenced this pull request Sep 7, 2026
* Document BarRobot v2 rewrite design

* Rewrite BarRobot around testable services

* Add BarRobot behavioral tests and CI

* Fix package installation in CI

* Design BarRobot v3 greenfield architecture

* Replace legacy app with BarRobot v3

* Add isolated GPIO motion service

* Add v3 tests deployment and CI

* Add product and S-curve motion profiles

* Add product changeover and party controls

* Slow lopsided automatic moves

* Add v2 UI screenshots to README (#5)

Capture the five Flask pages plus the pour status list against a demo
12-bottle configuration in safe mode, and document each in a new 'The UI'
section.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Add v3 UI screenshots to README (#6)

Capture the four console tabs plus the running and manual-step job states
against a demo 12-bottle cocktail profile, and document each in a new
'The UI' section.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Move the original Flask app into v1/

Makes room for the v2 and v3 implementations to live alongside it instead
of replacing it.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Index the three versions from the repository root

Adds a root README explaining what v1, v2, and v3 are and which to use,
merges the three .gitignore files, and splits CI into per-version
workflows that only run when their own directory changes. Version READMEs
now point at the shared LICENSE and hero image one level up.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Raise v2's Flask and requests floors past known CVEs

Codacy's dependency scanner resolves the lower bound of each constraint,
so ">=3.1" and ">=2.32" were reported as vulnerable:

  Flask 3.1     CVE-2025-47278  session signing fallback key
                CVE-2026-27205  session data cached improperly
  requests 2.32 CVE-2024-47081  .netrc credential leak via crafted URLs
                CVE-2026-25645  predictable temporary file creation

Both fixes land within the ranges the project already allows, so the
floors move to the first patched releases and the upper bounds stand.
ruff and the 31 tests pass against Flask 3.1.3 and requests 2.33.1.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Configure Codacy for the three-version layout

Codacy gates pull requests on new issues, and it decides what is new by
path. Moving v1 into v1/ and bringing v2/ and v3/ onto main therefore
presents both rewrites as brand-new code, so this PR inherits the whole
backlog of the two open rewrite PRs at once: 62 issues from #2 plus 30
from #3, against a limit of zero.

Four of those were real and are fixed in the preceding commit. The rest
are analysers reaching wrong conclusions because they run with their own
defaults rather than the configuration each version is built with:

  - Bandit reports B101 on every pytest assert and reads conftest.py's
    throwaway Flask signing key as a hardcoded password.
  - Codacy's ESLint has no access to v3's tsconfig, so it cannot see
    noUncheckedIndexedAccess and reports the undefined-guards that make
    v3 compile as conditions that are always false.
  - Opengrep flags a state repository whose path is configuration, and
    two innerHTML writes whose inputs are already escaped through the
    DOM's own textContent round-trip.
  - flawfinder flags every strncpy in motiond, all of which write
    memset-zeroed fixed buffers under an explicit sizeof - 1 bound.

Each exclusion names one analyser and one path and carries its reason.
No analyser is turned off repository-wide: v2's application code, 986
lines across eight files, stays under Bandit, and v3 keeps linting src/
in CI with typescript-eslint against the real tsconfig.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Update v3's lockfile past nineteen dependency CVEs

Codacy's dependency scanner reported these against package-lock.json.
They are new since PR #3 was last analysed, not something the
restructure introduced:

  @fastify/static 10.1.0  path traversal past route guards, and
                          authorization bypass via non-canonical paths
  fastify 5.10.0          request spoofing through numeric trustProxy,
                          and schema validation bypass on root primitives
  fast-uri 3.1.3, 4.1.0   six findings, mostly SSRF and host confusion
                          from parsing inconsistencies
  find-my-way 9.6.0       HTTP/2 denial of service
  brace-expansion 5.0.7   denial of service through memory exhaustion

Every fix was already inside the semver ranges package.json declares, so
this is a lockfile refresh with no manifest change: fastify 5.12.3,
@fastify/static 10.1.3, fast-uri 3.1.7 and 4.1.4, find-my-way 9.9.0,
brace-expansion 5.0.9. npm audit is clean, npm ci reproduces the tree,
and prettier, tsc, eslint, the build, and all 25 tests pass on it.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

* Cover the remaining analysers that re-litigate v2 and v3 style

The first pass cut Codacy's count on this PR from 92 to 45 and confirmed
the exclusions land, but four analysers were named wrongly or not at all:

  - The strncpy findings in motiond come from Opengrep as well as
    flawfinder; only flawfinder was listed.
  - The 0.0001 objection in web/app.js is PMD's InnaccurateNumericLiteral,
    not ESLint, so excluding ESLint there had no effect.
  - Prospector and Pylint report twenty-two docstring and layout findings
    across v2 under a convention that contradicts the ruff configuration
    v2 actually enforces in CI.

Each addition carries the same justification the existing entries do.

Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: cmc0619 <cmc0619@users.noreply.github.com>
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