feat: raw command execution, flow tracking, auto-off#271
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces raw hardware command execution capabilities and expands biometric data collection. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add POST /device/execute endpoint for power users to send allowlisted hardware commands with raw args passthrough. Includes disclaimer in response and ADR 0016 documenting rationale and risks.
Add flow_readings table to biometrics schema, recordFlowData() method to DeviceStateSync with 60s rate limiting, and GET /water-level/flow endpoint to query historical readings.
Add autoOffEnabled and autoOffMinutes columns to side_settings table, and accept them in the PATCH /settings/side endpoint. The actual presence detection watcher is future work.
- FrankenHardwareClient.getRawClient() returns null in production, so the execute endpoint was broken. Use sendCommand from dacTransport directly (same transport all other hardware calls use). - Add .limit(10080) to getFlowReadings to cap result size
ng
force-pushed
the
feature/hardware-features
branch
from
40058c7 to
feb9f97
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hardware/deviceStateSync.ts (1)
110-110:⚠️ Potential issue | 🟡 MinorMisleading comment: flowrate is not in "centidegrees".
The comment says
// centidegrees, matching bed_temp conventionbut flowrate is a volumetric rate (e.g., L/min), not a temperature. The*100conversion stores centi-units (hundredths), but the unit name in the comment is incorrect.📝 Suggested comment fix
- leftFlowrateCd: integer('left_flowrate_cd'), // centidegrees, matching bed_temp convention + leftFlowrateCd: integer('left_flowrate_cd'), // centi-units (flowrate × 100) for precision without floats🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hardware/deviceStateSync.ts` at line 110, The inline comment calling the flowrate "centidegrees" is wrong; update the comment near the flowrate write (where the value is multiplied by *100 and written alongside the this.lastWaterLevelWrite check) to state that the code stores flowrate as hundredths (centi-units) of the volumetric rate (e.g., hundredths of L/min) instead of "centidegrees", so it accurately describes the unit conversion.
🧹 Nitpick comments (4)
docs/adr/0016-raw-command-execution.md (1)
10-12: Consider documenting the allowed command names.The ADR mentions "command name allowlisting" but doesn't enumerate which commands are permitted. Documenting the allowlist here (SET_TEMP, SET_ALARM, ALARM_LEFT, ALARM_RIGHT, SET_SETTINGS, PRIME, DEVICE_STATUS, ALARM_CLEAR) would help future readers understand the scope without diving into code, and makes the ADR self-contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0016-raw-command-execution.md` around lines 10 - 12, Update the ADR for the POST /device/execute decision to explicitly list the allowed command names in the "Decision" or a new "Allowlist" section so the document is self-contained; include the exact allowed tokens SET_TEMP, SET_ALARM, ALARM_LEFT, ALARM_RIGHT, SET_SETTINGS, PRIME, DEVICE_STATUS, and ALARM_CLEAR and state that these are the only command names accepted by the endpoint (with the endpoint name POST /device/execute and the phrase "command name allowlisting" kept intact).src/server/routers/device.ts (2)
544-544: Consider adding a max length to theargsparameter.The
argsstring is passed verbatim to the hardware socket with no length limit. While this is a power-user feature, an extremely long string could cause memory pressure when buffered or trigger unexpected behavior at the protocol level.A reasonable upper bound (e.g., 1024 or 4096 characters) would add a minimal safety net without limiting legitimate use.
🛡️ Suggested change
- args: z.string().optional(), + args: z.string().max(4096).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routers/device.ts` at line 544, The args input in the device router is currently z.string().optional() and can be arbitrarily long; add a max length to prevent memory/protocol issues by changing the zod schema for args (the args parameter in the router definition) to include a .max(...) constraint (e.g., 1024 or 4096 chars) while preserving optionality, and update any relevant validation error message/handlers to surface that the args value is too long.
25-34: Consider deriving the allowlist from a single source of truth.The allowed command names are duplicated: once in
COMMAND_MAPkeys and again in thez.enum()at line 543. If these drift apart, the endpoint could accept a command name that isn't in the map (causinghwCommandto beundefined) or reject valid commands.♻️ Suggested approach
-const COMMAND_MAP: Record<string, HardwareCommand> = { +const COMMAND_MAP = { SET_TEMP: HardwareCommand.SET_TEMP, SET_ALARM: HardwareCommand.SET_ALARM, ALARM_LEFT: HardwareCommand.ALARM_LEFT, ALARM_RIGHT: HardwareCommand.ALARM_RIGHT, SET_SETTINGS: HardwareCommand.SET_SETTINGS, PRIME: HardwareCommand.PRIME, DEVICE_STATUS: HardwareCommand.DEVICE_STATUS, ALARM_CLEAR: HardwareCommand.ALARM_CLEAR, -} +} as const satisfies Record<string, HardwareCommand> + +const ALLOWED_COMMANDS = Object.keys(COMMAND_MAP) as [keyof typeof COMMAND_MAP, ...(keyof typeof COMMAND_MAP)[]]Then use in the schema:
command: z.enum(ALLOWED_COMMANDS),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routers/device.ts` around lines 25 - 34, COMMAND_MAP's keys and the z.enum allowlist are duplicated; create a single source of truth by deriving an ALLOWED_COMMANDS constant from the keys of COMMAND_MAP (e.g., capture Object.keys(COMMAND_MAP) as a readonly tuple/array and assert its literal types), then use z.enum(ALLOWED_COMMANDS) in the schema instead of repeating the strings; update any code that references the old enum list and ensure the handler that looks up hwCommand from COMMAND_MAP (hwCommand) will no longer get undefined because the schema only permits keys present in COMMAND_MAP.src/db/migrations/0002_ambitious_sir_ram.sql (1)
1-2: Add DB-level constraints for auto-off fields.
auto_off_minutesandauto_off_enabledare validated at API level, but the table schema currently allows out-of-contract values at the DB layer. Please enforce bounds/domains in the migration too.Proposed migration hardening
-ALTER TABLE `side_settings` ADD `auto_off_enabled` integer DEFAULT false NOT NULL;--> statement-breakpoint -ALTER TABLE `side_settings` ADD `auto_off_minutes` integer DEFAULT 30 NOT NULL; +ALTER TABLE `side_settings` + ADD `auto_off_enabled` integer DEFAULT false NOT NULL CHECK (`auto_off_enabled` IN (0, 1));--> statement-breakpoint +ALTER TABLE `side_settings` + ADD `auto_off_minutes` integer DEFAULT 30 NOT NULL CHECK (`auto_off_minutes` BETWEEN 5 AND 120);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/migrations/0002_ambitious_sir_ram.sql` around lines 1 - 2, The migration adds side_settings.auto_off_enabled and auto_off_minutes without DB-level domains; update the ALTER TABLE statements to enforce types and bounds by using a boolean/numeric default and CHECK constraints: set auto_off_enabled DEFAULT 0 NOT NULL CHECK (auto_off_enabled IN (0,1)) and set auto_off_minutes DEFAULT 30 NOT NULL CHECK (auto_off_minutes BETWEEN 0 AND 1440) (adjust the upper bound if your policy differs), keeping the same column names side_settings.auto_off_enabled and side_settings.auto_off_minutes so the DB will reject out-of-contract values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hardware/deviceStateSync.ts`:
- Around line 125-149: The recordFlowData method is never invoked; wire it into
the frame processing pipeline so frzHealth frames trigger a call to
DeviceStateSync.recordFlowData. Locate the frame handler (e.g., the function
that receives/parses incoming frames such as processFrame, handleFrame, or
onFrame) and when a frame is identified as a frzHealth payload (check the
frame.type or parsed object name), call this.recordFlowData(parsedFrzHealth)
where parsedFrzHealth matches the expected shape (left/right pump/temps). Ensure
the invocation is performed asynchronously-safe (no blocking) and does not
bypass the existing rate limit inside recordFlowData.
In `@src/server/routers/device.ts`:
- Around line 540-541: The execute publicProcedure (meta openapi POST path
'/device/execute') is currently exposed with protect: false; gate it behind a
debug/advanced feature flag (e.g., process.env.ENABLE_DEVICE_EXECUTE or a
Config.isAdvancedMode()) by changing the procedure to check that flag before
running (or attach a middleware that rejects requests when the flag is false)
and set meta.protect appropriately; update the execute handler to throw a
forbidden error when the feature flag is disabled so the endpoint is effectively
disabled in production unless the debug flag is enabled.
In `@src/server/routers/settings.ts`:
- Around line 189-190: The getAll response fallback objects in the settings
router are missing the new optional fields autoOffEnabled and autoOffMinutes,
causing inconsistent response shapes; update the fallback/default side objects
returned by getAll to include autoOffEnabled and autoOffMinutes (set them to
undefined or an appropriate default consistent with the zod schema) so the
response shape always contains these keys and matches the zod types for
autoOffEnabled and autoOffMinutes.
In `@src/server/routers/waterLevel.ts`:
- Around line 233-238: The DB query is returned without awaiting, so errors
escape the surrounding try/catch; modify the code that builds the query using
biometricsDb.select().from(flowReadings).where(gt(flowReadings.timestamp,
since)).orderBy(flowReadings.timestamp).limit(10080) to await the promise (i.e.,
use await with biometricsDb...) so any thrown error is caught by the existing
catch block; ensure the surrounding function (the procedure handling
flowReadings) remains async and keep existing error handling/logging intact.
---
Outside diff comments:
In `@src/hardware/deviceStateSync.ts`:
- Line 110: The inline comment calling the flowrate "centidegrees" is wrong;
update the comment near the flowrate write (where the value is multiplied by
*100 and written alongside the this.lastWaterLevelWrite check) to state that the
code stores flowrate as hundredths (centi-units) of the volumetric rate (e.g.,
hundredths of L/min) instead of "centidegrees", so it accurately describes the
unit conversion.
---
Nitpick comments:
In `@docs/adr/0016-raw-command-execution.md`:
- Around line 10-12: Update the ADR for the POST /device/execute decision to
explicitly list the allowed command names in the "Decision" or a new "Allowlist"
section so the document is self-contained; include the exact allowed tokens
SET_TEMP, SET_ALARM, ALARM_LEFT, ALARM_RIGHT, SET_SETTINGS, PRIME,
DEVICE_STATUS, and ALARM_CLEAR and state that these are the only command names
accepted by the endpoint (with the endpoint name POST /device/execute and the
phrase "command name allowlisting" kept intact).
In `@src/db/migrations/0002_ambitious_sir_ram.sql`:
- Around line 1-2: The migration adds side_settings.auto_off_enabled and
auto_off_minutes without DB-level domains; update the ALTER TABLE statements to
enforce types and bounds by using a boolean/numeric default and CHECK
constraints: set auto_off_enabled DEFAULT 0 NOT NULL CHECK (auto_off_enabled IN
(0,1)) and set auto_off_minutes DEFAULT 30 NOT NULL CHECK (auto_off_minutes
BETWEEN 0 AND 1440) (adjust the upper bound if your policy differs), keeping the
same column names side_settings.auto_off_enabled and
side_settings.auto_off_minutes so the DB will reject out-of-contract values.
In `@src/server/routers/device.ts`:
- Line 544: The args input in the device router is currently
z.string().optional() and can be arbitrarily long; add a max length to prevent
memory/protocol issues by changing the zod schema for args (the args parameter
in the router definition) to include a .max(...) constraint (e.g., 1024 or 4096
chars) while preserving optionality, and update any relevant validation error
message/handlers to surface that the args value is too long.
- Around line 25-34: COMMAND_MAP's keys and the z.enum allowlist are duplicated;
create a single source of truth by deriving an ALLOWED_COMMANDS constant from
the keys of COMMAND_MAP (e.g., capture Object.keys(COMMAND_MAP) as a readonly
tuple/array and assert its literal types), then use z.enum(ALLOWED_COMMANDS) in
the schema instead of repeating the strings; update any code that references the
old enum list and ensure the handler that looks up hwCommand from COMMAND_MAP
(hwCommand) will no longer get undefined because the schema only permits keys
present in COMMAND_MAP.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8041eec2-211b-4c3b-bf65-657bd94ee6b0
📒 Files selected for processing (13)
docs/adr/0016-raw-command-execution.mdsrc/db/biometrics-migrations/0006_tired_gressill.sqlsrc/db/biometrics-migrations/meta/0006_snapshot.jsonsrc/db/biometrics-migrations/meta/_journal.jsonsrc/db/biometrics-schema.tssrc/db/migrations/0002_ambitious_sir_ram.sqlsrc/db/migrations/meta/0002_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/hardware/deviceStateSync.tssrc/server/routers/device.tssrc/server/routers/settings.tssrc/server/routers/waterLevel.ts
Comment on lines
+125
to
+149
| /** Write flow/pump data to biometrics DB, rate-limited to once per 60s. */ | ||
| recordFlowData(frzHealth: { | ||
| left: { pump: { rpm: number }, temps: { flowrate: number } } | ||
| right: { pump: { rpm: number }, temps: { flowrate: number } } | ||
| }): void { | ||
| const now = Date.now() | ||
| if (now - this.lastFlowWrite < 60_000) return | ||
|
|
||
| try { | ||
| biometricsDb | ||
| .insert(flowReadings) | ||
| .values({ | ||
| timestamp: new Date(now), | ||
| leftFlowrateCd: Math.round(frzHealth.left.temps.flowrate * 100), | ||
| rightFlowrateCd: Math.round(frzHealth.right.temps.flowrate * 100), | ||
| leftPumpRpm: frzHealth.left.pump.rpm, | ||
| rightPumpRpm: frzHealth.right.pump.rpm, | ||
| }) | ||
| .run() | ||
| this.lastFlowWrite = now | ||
| } | ||
| catch (error) { | ||
| console.error('DeviceStateSync: failed to write flow readings:', error instanceof Error ? error.message : error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to recordFlowData to verify it's actually used
rg -n 'recordFlowData' --type tsRepository: sleepypod/core
Length of output: 123
Wire up recordFlowData to the frame processing pipeline.
The method is defined but never called anywhere in the codebase. Ensure it's invoked when frzHealth frames are processed to fulfill the intended functionality of persisting flow data at 60-second intervals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hardware/deviceStateSync.ts` around lines 125 - 149, The recordFlowData
method is never invoked; wire it into the frame processing pipeline so frzHealth
frames trigger a call to DeviceStateSync.recordFlowData. Locate the frame
handler (e.g., the function that receives/parses incoming frames such as
processFrame, handleFrame, or onFrame) and when a frame is identified as a
frzHealth payload (check the frame.type or parsed object name), call
this.recordFlowData(parsedFrzHealth) where parsedFrzHealth matches the expected
shape (left/right pump/temps). Ensure the invocation is performed
asynchronously-safe (no blocking) and does not bypass the existing rate limit
inside recordFlowData.
Comment on lines
+540
to
+541
| execute: publicProcedure | ||
| .meta({ openapi: { method: 'POST', path: '/device/execute', protect: false, tags: ['Device'] } }) |
There was a problem hiding this comment.
Missing debug/advanced mode gating as mentioned in requirements.
The PR objectives from issue #191 specify: "Gate access behind a debug/advanced mode flag (intended for power users/debugging, not normal app usage)." The current implementation has protect: false with no additional access control.
Consider whether this endpoint should be conditionally enabled (e.g., via environment variable or config flag) to prevent accidental exposure in production builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/routers/device.ts` around lines 540 - 541, The execute
publicProcedure (meta openapi POST path '/device/execute') is currently exposed
with protect: false; gate it behind a debug/advanced feature flag (e.g.,
process.env.ENABLE_DEVICE_EXECUTE or a Config.isAdvancedMode()) by changing the
procedure to check that flag before running (or attach a middleware that rejects
requests when the flag is false) and set meta.protect appropriately; update the
execute handler to throw a forbidden error when the feature flag is disabled so
the endpoint is effectively disabled in production unless the debug flag is
enabled.
- Add missing `await` in getFlowReadings so errors hit the catch block - Add autoOffEnabled/autoOffMinutes to settings side fallback defaults - Wire recordFlowData into frzHealth frame pipeline via onServerFrame - Filter server-side listeners to frzHealth only (avoid hot-path overhead) - Store unsubscribe handle and clean up on shutdown
4 tasks
ng
added a commit
that referenced
this pull request
Apr 1, 2026
## Summary - feat: 24/7 temperature mode with keepalive and stale display fix (#300) - chore: add db:reset scripts for convenient database resets (#299) - feat: track flowrate and pump RPM with anomaly detection (#302) - feat: scheduler fixes and new scheduling features (#270) - feat: raw command execution, flow tracking, auto-off (#271)
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /device/executeendpoint for power users to send raw hardware commands by name. Includes ADR (0016) and disclaimer. This is an unsupported power user/debug feature with no validation on command arguments.flow_readingstable in biometrics DB.DeviceStateSyncpersists flow data fromfrzHealthframes at 60s intervals. NewGET /water-level/flowAPI endpoint for querying historical flow data.autoOffEnabledandautoOffMinutescolumns onside_settings. Settings API updated. (Watcher service is future work.)Closes #191, #241, #257
Test plan
Summary by CodeRabbit
New Features
Documentation