Skip to content

Rewrite BarRobot v2 around safe, testable services - #2

Draft
cmc0619 wants to merge 5 commits into
mainfrom
agent/barrobot-v2-rewrite
Draft

cmc0619 wants to merge 5 commits into
mainfrom
agent/barrobot-v2-rewrite

Conversation

@cmc0619

@cmc0619 cmc0619 commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Summary

Rewrites BarRobot v2 around explicit configuration, recipe, planning, web, hardware-controller, and GPIO-adapter boundaries while preserving the existing route URLs, 12-slot workflow, recipe-cache schema, pantry/substitution behavior, safe mode, configurable pins, shortest-path rotation, and conservative motion ramp.

The first commit contains the complete review and behavior contract in design.md. Intentional safety and correctness changes are documented there rather than hidden in refactoring.

Important behavior changes

  • migrates the shipped bottle_config.json into canonical config.json
  • makes configured shot_size determine actuator repetitions
  • preflights the complete recipe before any ingredient is poured
  • serializes complete hardware operations
  • uses POST for the menu's Pour action while retaining legacy GET compatibility
  • requires an operator-established position before live movement
  • distributes all 1,600 microsteps exactly around 12 slots
  • invalidates position after simulated safe-mode movement or interrupted live movement
  • disables Flask debug/reloader behavior by default

Developer and operator impact

  • fresh-clone dependency and sample-config files now exist
  • Raspberry Pi GPIO is lazy and injectable, so development and CI require no hardware
  • the root app.py and hardware.py remain compatibility entry points
  • README commissioning steps call out shot calibration and trusted-position setup
  • version advances to 2.0.0 because the documented safety changes are intentionally breaking at the hardware boundary

Validation

  • ruff format --check .
  • ruff check .
  • pytest — 31 passed
  • python -m compileall -q app.py hardware.py barrobot tests
  • git diff --check

No tests contact CocktailDB or emit Raspberry Pi GPIO.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b8f4eea8-7821-4cba-821c-ed6648c13beb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

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>
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

The PR rewrites BarRobot around validated, injectable service boundaries.

  • Introduces atomic configuration and recipe-cache persistence with legacy configuration migration.
  • Adds preflighted shot-size-aware drink planning and serialized GPIO control.
  • Adds trusted-position handling, exact slot-step distribution, safe-mode transitions, and configurable hardware adapters.
  • Rebuilds the Flask routes, templates, deployment files, documentation, CI, and test suite around the v2 architecture.

Confidence Score: 2/5

This PR should not merge until actuator deassertion is exception-safe and GET requests can no longer initiate a physical pour.

A mid-press exception can leave the valve energized, while the compatibility GET route still reaches the complete live rotation-and-dispense path without an intentional POST action.

Files Needing Attention: barrobot/hardware.py, barrobot/routes.py

Important Files Changed

Filename Overview
barrobot/hardware.py Adds serialized motion and actuator control, but actuator presses are not exception-safe and can leave the valve energized.
barrobot/routes.py Adds service-oriented web workflows and complete-plan locking, but the retained GET pour route can still initiate physical dispensing.
barrobot/planning.py Adds immutable preflighted automatic/manual actions and shot-size-aware repetition calculation.
barrobot/config.py Adds normalized validation, legacy migration, and atomic configuration persistence.
barrobot/recipes.py Adds typed CocktailDB conversion, explicit download errors, and atomic cache persistence.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    UI[Flask routes and templates] --> Config[ConfigStore]
    UI --> Recipes[RecipeRepository]
    UI --> Planner[Drink planner]
    Config --> Planner
    Recipes --> Planner
    Planner --> Controller[HardwareController]
    Controller --> GPIO[GPIO adapter]
    GPIO --> Hardware[Turret and actuator]
Loading

Fix All in Claude Code Fix All in Codex

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

Comment thread barrobot/hardware.py
Comment on lines +126 to +128
self._output("ACTUATOR", True)
self._sleeper(PUSH_DURATION_MS / 1000)
self._output("ACTUATOR", False)

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 Actuator remains energized on failure

When the sleeper or a GPIO write raises after ACTUATOR is set high, this loop exits without reliably setting it low, causing the bottle valve to remain open and continue dispensing until cleanup or operator intervention.

Fix in Claude Code Fix in Codex

Comment thread barrobot/routes.py
return jsonify(status="ok", slot=slot)


@web.route("/make_drink/<name>", methods=["GET", "POST"])

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 GET still executes physical pours

When a crawler, prefetch, refresh, or embedded URL sends GET for a known recipe after live position is established, the shared route body rotates the turret and presses the actuator, causing a drink to be dispensed without an intentional Pour submission.

Suggested change
@web.route("/make_drink/<name>", methods=["GET", "POST"])
@web.post("/make_drink/<name>")

Fix in Claude Code Fix in Codex

@codacy-production

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 57 high · 3 medium · 2 minor

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

Results:
62 new issues

Category Results
Security 3 medium
2 minor
57 high

View in Codacy

🟢 Metrics 283 complexity · 2 duplication

Metric Results
Complexity 283
Duplication 2

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.

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>
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