From 3ed49ee882e81b93750208db367f162fdaffcdaa Mon Sep 17 00:00:00 2001 From: HomeOS Bootstrap Date: Sat, 18 Jul 2026 01:15:22 +0200 Subject: [PATCH] fix(automations): honor conditions + atomic cooldown claim (R15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex R15 flagged two P1 correctness bugs in AutomationEngine: - Conditions were never evaluated — a trigger→condition→action rule ran its actions unconditionally. Now every stored condition must match current entity state (AND) before actions run; a false condition does NOT touch the cooldown clock, so the rule stays armed for its next legitimate trigger. - Cooldown was read-check-then-save, so concurrent queue workers on a burst of state changes could all pass the check and each fire. Now claimed with a single conditional UPDATE — exactly one worker wins the race (H5). +3 tests (condition true/false, cooldown-not-armed-on-failed-condition). AutomationTest 9 green. Co-Authored-By: Claude Opus 4.8 --- .reviews/r15-d10172a.md | 1184 +++++++++++++++++++++++++++++ app/Services/AutomationEngine.php | 72 +- tests/Feature/AutomationTest.php | 72 ++ 3 files changed, 1319 insertions(+), 9 deletions(-) create mode 100644 .reviews/r15-d10172a.md diff --git a/.reviews/r15-d10172a.md b/.reviews/r15-d10172a.md new file mode 100644 index 0000000..faf5b89 --- /dev/null +++ b/.reviews/r15-d10172a.md @@ -0,0 +1,1184 @@ +OpenAI Codex v0.144.5 +-------- +workdir: /home/nexxo/homeos +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f725a-883f-7863-92a3-eec5b0fdcd43 +-------- +user +commit d10172a +warning: Codex could not find bubblewrap on PATH. Install bubblewrap with your OS package manager. See the sandbox prerequisites: https://developers.openai.com/codex/concepts/sandboxing#prerequisites. Codex will use the bundled bubblewrap in the meantime. +exec +/bin/bash -lc 'git status --short && git show --stat --oneline --decorate --no-renames d10172a && git diff d10172a''^ d10172a --no-ext-diff --unified=80 --no-renames' in /home/nexxo/homeos + succeeded in 0ms: +?? .reviews/ +d10172a (HEAD -> feat/phase-1-bootstrap, origin/feat/phase-1-bootstrap) feat(automations): trigger→condition→action engine with cooldown + dry-run + app/Console/Commands/AutomationsTickCommand.php | 21 +++ + app/Listeners/EvaluateAutomations.php | 18 +++ + app/Livewire/Modals/CreateAutomation.php | 129 ++++++++++++++++ + app/Models/Automation.php | 3 +- + app/Providers/AppServiceProvider.php | 11 +- + app/Services/AutomationEngine.php | 95 ++++++++++++ + ...026_07_18_040001_add_dry_run_to_automations.php | 23 +++ + lang/de/automations.php | 19 +++ + lang/en/automations.php | 19 +++ + .../views/livewire/automations/index.blade.php | 9 +- + .../livewire/modals/create-automation.blade.php | 119 +++++++++++++++ + routes/console.php | 3 + + tests/Feature/AutomationTest.php | 168 +++++++++++++++++++++ + 13 files changed, 628 insertions(+), 9 deletions(-) +diff --git a/app/Console/Commands/AutomationsTickCommand.php b/app/Console/Commands/AutomationsTickCommand.php +new file mode 100644 +index 0000000..ab392d3 +--- /dev/null ++++ b/app/Console/Commands/AutomationsTickCommand.php +@@ -0,0 +1,21 @@ ++tick(); ++ ++ return self::SUCCESS; ++ } ++} +diff --git a/app/Listeners/EvaluateAutomations.php b/app/Listeners/EvaluateAutomations.php +new file mode 100644 +index 0000000..513a93a +--- /dev/null ++++ b/app/Listeners/EvaluateAutomations.php +@@ -0,0 +1,18 @@ ++engine->onStateChange($event->deviceUuid, $event->entityKey, $event->state); ++ } ++} +diff --git a/app/Livewire/Modals/CreateAutomation.php b/app/Livewire/Modals/CreateAutomation.php +new file mode 100644 +index 0000000..533094e +--- /dev/null ++++ b/app/Livewire/Modals/CreateAutomation.php +@@ -0,0 +1,129 @@ ++ */ ++ public function getEntityOptionsProperty(): array ++ { ++ return Entity::with('device')->get() ++ ->filter(fn (Entity $e) => $e->device !== null) ++ ->map(fn (Entity $e) => [ ++ 'value' => $e->device->uuid.'|'.$e->key, ++ 'label' => trim(($e->device->name ?? '?').' · '.($e->name ?: $e->key)), ++ 'controllable' => in_array($e->type, ['switch', 'light'], true), ++ ]) ++ ->sortBy('label', SORT_NATURAL | SORT_FLAG_CASE) ++ ->values()->all(); ++ } ++ ++ public function save() ++ { ++ $rules = [ ++ 'name' => 'required|string|max:100', ++ 'triggerType' => 'required|in:state_change,time', ++ 'actionType' => 'required|in:switch,notify', ++ 'cooldownSeconds' => 'integer|min:0|max:86400', ++ ]; ++ ++ if ($this->triggerType === 'state_change') { ++ $rules['triggerEntity'] = 'required|string'; ++ } else { ++ $rules['triggerAt'] = ['required', 'regex:/^\d{2}:\d{2}$/']; ++ } ++ ++ if ($this->actionType === 'switch') { ++ $rules['actionEntity'] = 'required|string'; ++ } else { ++ $rules['actionMessage'] = 'required|string|max:200'; ++ } ++ ++ $this->validate($rules); ++ ++ Automation::create([ ++ 'name' => $this->name, ++ 'enabled' => true, ++ 'trigger_type' => $this->triggerType, ++ 'trigger_config' => $this->buildTriggerConfig(), ++ 'conditions' => [], ++ 'actions' => [$this->buildAction()], ++ 'cooldown_seconds' => $this->cooldownSeconds, ++ 'dry_run' => $this->dryRun, ++ ]); ++ ++ $this->closeModal(); ++ ++ return redirect()->route('automations.index'); ++ } ++ ++ /** @return array */ ++ private function buildTriggerConfig(): array ++ { ++ if ($this->triggerType === 'time') { ++ return ['at' => $this->triggerAt]; ++ } ++ ++ [$device, $key] = $this->split($this->triggerEntity); ++ ++ return ['device' => $device, 'entity_key' => $key, 'field' => 'on', 'equals' => $this->triggerEquals === '1']; ++ } ++ ++ /** @return array */ ++ private function buildAction(): array ++ { ++ if ($this->actionType === 'notify') { ++ return ['type' => 'notify', 'message' => $this->actionMessage]; ++ } ++ ++ [$device, $key] = $this->split($this->actionEntity); ++ ++ return ['type' => 'switch', 'device' => $device, 'entity_key' => $key, 'on' => $this->actionOn === '1']; ++ } ++ ++ /** @return array{0:?string,1:?string} */ ++ private function split(string $value): array ++ { ++ $parts = explode('|', $value, 2); ++ ++ return [$parts[0] ?? null, $parts[1] ?? null]; ++ } ++ ++ public function render() ++ { ++ return view('livewire.modals.create-automation'); ++ } ++} +diff --git a/app/Models/Automation.php b/app/Models/Automation.php +index 75d4919..9ffcaee 100644 +--- a/app/Models/Automation.php ++++ b/app/Models/Automation.php +@@ -1,24 +1,25 @@ + 'boolean', ++ 'dry_run' => 'boolean', + 'trigger_config' => 'array', + 'conditions' => 'array', + 'actions' => 'array', + 'last_triggered_at' => 'datetime', + ]; + } +diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php +index 452e6b6..68c7520 100644 +--- a/app/Providers/AppServiceProvider.php ++++ b/app/Providers/AppServiceProvider.php +@@ -1,24 +1,21 @@ + where('trigger_type', 'state_change')->get() ++ ->each(function (Automation $automation) use ($deviceUuid, $entityKey, $state) { ++ if ($this->matchesStateTrigger($automation, $deviceUuid, $entityKey, $state)) { ++ $this->maybeRun($automation); ++ } ++ }); ++ } ++ ++ /** Evaluate time-based automations (called every minute by the scheduler). */ ++ public function tick(): void ++ { ++ $hhmm = now()->format('H:i'); ++ ++ Automation::where('enabled', true)->where('trigger_type', 'time')->get() ++ ->each(function (Automation $automation) use ($hhmm) { ++ if (data_get($automation->trigger_config, 'at') === $hhmm) { ++ $this->maybeRun($automation); ++ } ++ }); ++ } ++ ++ private function matchesStateTrigger(Automation $automation, string $deviceUuid, string $entityKey, array $state): bool ++ { ++ $cfg = $automation->trigger_config ?? []; ++ ++ if (($cfg['device'] ?? null) !== $deviceUuid || ($cfg['entity_key'] ?? null) !== $entityKey) { ++ return false; ++ } ++ ++ $field = $cfg['field'] ?? 'on'; ++ ++ return array_key_exists($field, $state) && $state[$field] == ($cfg['equals'] ?? true); ++ } ++ ++ private function maybeRun(Automation $automation): void ++ { ++ if ($automation->cooldown_seconds > 0 ++ && $automation->last_triggered_at !== null ++ && $automation->last_triggered_at->gt(now()->subSeconds($automation->cooldown_seconds))) { ++ return; ++ } ++ ++ foreach ($automation->actions ?? [] as $action) { ++ $this->runAction($automation, $action); ++ } ++ ++ $automation->forceFill(['last_triggered_at' => now()])->save(); ++ } ++ ++ private function runAction(Automation $automation, array $action): void ++ { ++ $type = $action['type'] ?? null; ++ ++ if ($type === 'switch') { ++ $entity = Entity::query() ++ ->whereHas('device', fn ($q) => $q->where('uuid', $action['device'] ?? null)) ++ ->where('key', $action['entity_key'] ?? null) ++ ->with('device', 'state')->first(); ++ ++ if ($entity === null) { ++ return; ++ } ++ ++ if ($automation->dry_run) { ++ Log::info("[automation dry-run] {$automation->name}: switch {$entity->key}", $action); ++ ++ return; ++ } ++ ++ $this->commands->setOn($entity, (bool) ($action['on'] ?? true), 'automation'); ++ } elseif ($type === 'notify') { ++ Log::info("[automation".($automation->dry_run ? ' dry-run' : '')."] {$automation->name}: ".($action['message'] ?? '')); ++ } ++ } ++} +diff --git a/database/migrations/2026_07_18_040001_add_dry_run_to_automations.php b/database/migrations/2026_07_18_040001_add_dry_run_to_automations.php +new file mode 100644 +index 0000000..644ed24 +--- /dev/null ++++ b/database/migrations/2026_07_18_040001_add_dry_run_to_automations.php +@@ -0,0 +1,23 @@ ++boolean('dry_run')->default(false)->after('cooldown_seconds'); ++ }); ++ } ++ ++ public function down(): void ++ { ++ Schema::table('automations', function (Blueprint $table) { ++ $table->dropColumn('dry_run'); ++ }); ++ } ++}; +diff --git a/lang/de/automations.php b/lang/de/automations.php +index 0f3aa0e..b6ae031 100644 +--- a/lang/de/automations.php ++++ b/lang/de/automations.php +@@ -1,9 +1,28 @@ + 'Regeln: Auslöser → Bedingung → Aktion.', + 'empty_title' => 'Noch keine Automationen', + 'empty_body' => 'Erstelle Regeln, die bei Zustandsänderungen oder zu bestimmten Zeiten Geräte schalten oder benachrichtigen.', + 'trigger_state_change' => 'Bei Zustandsänderung', + 'trigger_time' => 'Zeitgesteuert', ++ 'add' => 'Automation erstellen', ++ 'add_title' => 'Neue Automation', ++ 'name' => 'Name', ++ 'trigger' => 'Auslöser', ++ 'trigger_entity' => 'Gerät / Entität', ++ 'trigger_equals' => 'Zustand', ++ 'trigger_at' => 'Uhrzeit', ++ 'action' => 'Aktion', ++ 'action_switch' => 'Gerät schalten', ++ 'action_notify' => 'Benachrichtigen', ++ 'action_entity' => 'Ziel-Gerät', ++ 'action_on' => 'Schalten auf', ++ 'message' => 'Nachricht', ++ 'cooldown' => 'Abklingzeit (Sekunden)', ++ 'cooldown_hint' => 'Mindestabstand zwischen zwei Auslösungen. 0 = keine.', ++ 'dry_run' => 'Testlauf', ++ 'dry_run_hint' => 'Protokolliert die Aktion, ohne wirklich zu schalten.', ++ 'state_on' => 'An', ++ 'state_off' => 'Aus', + ]; +diff --git a/lang/en/automations.php b/lang/en/automations.php +index 826359d..ecdebb7 100644 +--- a/lang/en/automations.php ++++ b/lang/en/automations.php +@@ -1,9 +1,28 @@ + 'Rules: trigger → condition → action.', + 'empty_title' => 'No automations yet', + 'empty_body' => 'Create rules that switch devices or notify on state changes or at set times.', + 'trigger_state_change' => 'On state change', + 'trigger_time' => 'Time-based', ++ 'add' => 'Create automation', ++ 'add_title' => 'New automation', ++ 'name' => 'Name', ++ 'trigger' => 'Trigger', ++ 'trigger_entity' => 'Device / entity', ++ 'trigger_equals' => 'State', ++ 'trigger_at' => 'Time', ++ 'action' => 'Action', ++ 'action_switch' => 'Switch device', ++ 'action_notify' => 'Notify', ++ 'action_entity' => 'Target device', ++ 'action_on' => 'Switch to', ++ 'message' => 'Message', ++ 'cooldown' => 'Cooldown (seconds)', ++ 'cooldown_hint' => 'Minimum gap between two runs. 0 = none.', ++ 'dry_run' => 'Dry run', ++ 'dry_run_hint' => 'Logs the action without actually switching.', ++ 'state_on' => 'On', ++ 'state_off' => 'Off', + ]; +diff --git a/resources/views/livewire/automations/index.blade.php b/resources/views/livewire/automations/index.blade.php +index f01c648..1a83293 100644 +--- a/resources/views/livewire/automations/index.blade.php ++++ b/resources/views/livewire/automations/index.blade.php +@@ -1,32 +1,39 @@ +
+- ++ ++ ++ ++ ++ + +
+ @if ($automations->isEmpty()) + +
+ +
+
{{ __('automations.empty_title') }}
+
{{ __('automations.empty_body') }}
+
+
+ + @else + +
+ @foreach ($automations as $a) +
+ +
+
{{ $a->name }}
+
{{ __('automations.trigger_'.$a->trigger_type) }}
+
+ +
+ @endforeach +
+ + @endif +
+
+diff --git a/resources/views/livewire/modals/create-automation.blade.php b/resources/views/livewire/modals/create-automation.blade.php +new file mode 100644 +index 0000000..8406e69 +--- /dev/null ++++ b/resources/views/livewire/modals/create-automation.blade.php +@@ -0,0 +1,119 @@ ++
++
++ ++

{{ __('automations.add_title') }}

++ ++
++ ++
++
++ ++ ++ @error('name')

{{ $message }}

@enderror ++
++ ++ {{-- Trigger --}} ++
++ {{ __('automations.trigger') }} ++ ++
++ ++
++ ++ @if ($triggerType === 'state_change') ++
++ ++ ++ @error('triggerEntity')

{{ $message }}

@enderror ++
++
++ ++ ++
++ @else ++
++ ++ ++ @error('triggerAt')

{{ $message }}

@enderror ++
++ @endif ++
++ ++ {{-- Action --}} ++
++ {{ __('automations.action') }} ++ ++
++ ++
++ ++ @if ($actionType === 'switch') ++
++ ++ ++ @error('actionEntity')

{{ $message }}

@enderror ++
++
++ ++ ++
++ @else ++
++ ++ ++ @error('actionMessage')

{{ $message }}

@enderror ++
++ @endif ++
++ ++
++ ++ ++

{{ __('automations.cooldown_hint') }}

++ @error('cooldownSeconds')

{{ $message }}

@enderror ++
++ ++ ++ ++
++ ++ ++
++
++
+diff --git a/routes/console.php b/routes/console.php +index eda4d86..ae228e1 100644 +--- a/routes/console.php ++++ b/routes/console.php +@@ -1,15 +1,18 @@ + comment(Inspiring::quote()); + })->purpose('Display an inspiring quote'); + + // Presence sweep — "home" immediately on association, "away" after a debounce (in the command). + Schedule::command('presence:poll')->everyMinute()->withoutOverlapping(); + + // Metrics sample for the charts (MQTT throughput + host CPU/memory). + Schedule::command('metrics:sample')->everyMinute()->withoutOverlapping(); ++ ++// Time-triggered automations (state-change ones fire off the DeviceStateChanged listener). ++Schedule::command('automations:tick')->everyMinute()->withoutOverlapping(); +diff --git a/tests/Feature/AutomationTest.php b/tests/Feature/AutomationTest.php +new file mode 100644 +index 0000000..86c8e5b +--- /dev/null ++++ b/tests/Feature/AutomationTest.php +@@ -0,0 +1,168 @@ ++app->bind(ShellyMqttDriver::class, fn () => new class implements DeviceDriver ++ { ++ public function turnOn(Entity $entity): void {} ++ ++ public function turnOff(Entity $entity): void {} ++ ++ public function setState(Entity $entity, array $state): void {} ++ ++ public function setLight(Entity $entity, array $params): void {} ++ ++ public function reboot(Device $device): void {} ++ ++ public function capabilities(): array ++ { ++ return []; ++ } ++ }); ++ } ++ ++ /** @return array{0:Device,1:Entity} */ ++ private function makeSwitch(): array ++ { ++ $device = Device::create(['name' => 'Lampe', 'vendor' => 'Shelly', 'protocol' => 'mqtt', 'status' => 'active']); ++ $entity = Entity::create(['device_id' => $device->id, 'type' => 'switch', 'key' => 'switch:0']); ++ DeviceState::create(['entity_id' => $entity->id, 'state' => ['on' => true]]); ++ ++ return [$device, $entity]; ++ } ++ ++ public function test_state_change_trigger_runs_switch_action_and_audits_as_automation(): void ++ { ++ [$trigger, $triggerEntity] = $this->makeSwitch(); ++ [$target, $targetEntity] = $this->makeSwitch(); ++ ++ Automation::create([ ++ 'name' => 'Motion turns light off', ++ 'enabled' => true, ++ 'trigger_type' => 'state_change', ++ 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], ++ 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], ++ 'cooldown_seconds' => 0, ++ ]); ++ ++ app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); ++ ++ $this->assertDatabaseHas('commands', ['source' => 'automation', 'command' => 'turn_off', 'result' => 'ok']); ++ } ++ ++ public function test_trigger_does_not_fire_when_value_differs(): void ++ { ++ [$trigger, $triggerEntity] = $this->makeSwitch(); ++ [$target, $targetEntity] = $this->makeSwitch(); ++ ++ Automation::create([ ++ 'name' => 'On only', ++ 'enabled' => true, ++ 'trigger_type' => 'state_change', ++ 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], ++ 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], ++ 'cooldown_seconds' => 0, ++ ]); ++ ++ app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => false]); ++ ++ $this->assertDatabaseCount('commands', 0); ++ } ++ ++ public function test_cooldown_prevents_a_second_run(): void ++ { ++ [$trigger, $triggerEntity] = $this->makeSwitch(); ++ [$target, $targetEntity] = $this->makeSwitch(); ++ ++ Automation::create([ ++ 'name' => 'Debounced', ++ 'enabled' => true, ++ 'trigger_type' => 'state_change', ++ 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], ++ 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], ++ 'cooldown_seconds' => 300, ++ ]); ++ ++ $engine = app(AutomationEngine::class); ++ $engine->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); ++ $engine->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); ++ ++ $this->assertDatabaseCount('commands', 1); ++ } ++ ++ public function test_dry_run_does_not_issue_a_command(): void ++ { ++ [$trigger, $triggerEntity] = $this->makeSwitch(); ++ [$target, $targetEntity] = $this->makeSwitch(); ++ ++ Automation::create([ ++ 'name' => 'Dry', ++ 'enabled' => true, ++ 'trigger_type' => 'state_change', ++ 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], ++ 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], ++ 'cooldown_seconds' => 0, ++ 'dry_run' => true, ++ ]); ++ ++ app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); ++ ++ $this->assertDatabaseCount('commands', 0); ++ } ++ ++ public function test_time_trigger_fires_on_matching_minute(): void ++ { ++ [$target, $targetEntity] = $this->makeSwitch(); ++ ++ Automation::create([ ++ 'name' => 'Morning light', ++ 'enabled' => true, ++ 'trigger_type' => 'time', ++ 'trigger_config' => ['at' => now()->format('H:i')], ++ 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => true]], ++ 'cooldown_seconds' => 0, ++ ]); ++ ++ app(AutomationEngine::class)->tick(); ++ ++ $this->assertDatabaseHas('commands', ['source' => 'automation', 'command' => 'turn_on']); ++ } ++ ++ public function test_disabled_automation_never_runs(): void ++ { ++ [$trigger, $triggerEntity] = $this->makeSwitch(); ++ [$target, $targetEntity] = $this->makeSwitch(); ++ ++ Automation::create([ ++ 'name' => 'Off', ++ 'enabled' => false, ++ 'trigger_type' => 'state_change', ++ 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], ++ 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], ++ 'cooldown_seconds' => 0, ++ ]); ++ ++ app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); ++ ++ $this->assertDatabaseCount('commands', 0); ++ } ++} + +exec +/bin/bash -lc "sed -n '1,160p' app/Events/DeviceStateChanged.php; sed -n '1,240p' app/Services/DeviceCommandService.php; grep -R \"DeviceStateChanged\" -n app tests | head -30; grep -R \"conditions\" -n app database tests | head -50; cat database/migrations/*create_automations*" in /home/nexxo/homeos + exited 1 in 0ms: + $this->deviceUuid, + 'entity' => $this->entityKey, + 'state' => $this->state, + ]; + } +} +state?->state, 'on', false); + + return $this->setOn($entity, ! $current, $source); + } + + public function setOn(Entity $entity, bool $on, string $source = 'user'): Command + { + return $this->run( + $entity->device, + $entity, + $on ? 'turn_on' : 'turn_off', + ['on' => $on], + $source, + fn (DeviceDriver $driver) => $on ? $driver->turnOn($entity) : $driver->turnOff($entity), + ); + } + + public function reboot(Device $device, string $source = 'user'): Command + { + return $this->run($device, null, 'reboot', [], $source, fn (DeviceDriver $driver) => $driver->reboot($device)); + } + + public function setLight(Entity $entity, array $params, string $source = 'user'): Command + { + return $this->run($entity->device, $entity, 'set_light', $params, $source, fn (DeviceDriver $driver) => $driver->setLight($entity, $params)); + } + + private function run(Device $device, ?Entity $entity, string $command, array $payload, string $source, callable $action): Command + { + $result = 'ok'; + + try { + $action($this->driverFor($device)); + } catch (\Throwable $e) { + report($e); + $result = 'error: '.$e->getMessage(); + } + + return Command::create([ + 'device_id' => $device->id, + 'entity_id' => $entity?->id, + 'source' => $source, + 'user_id' => Auth::id(), + 'command' => $command, + 'payload' => $payload, + 'result' => $result, + ]); + } + + private function driverFor(Device $device): DeviceDriver + { + return match (true) { + $device->vendor === 'Shelly' && $device->protocol === 'mqtt' => app(ShellyMqttDriver::class), + default => throw new \RuntimeException("No driver for {$device->vendor}/{$device->protocol}."), + }; + } +} +app/Listeners/EvaluateAutomations.php:5:use App\Events\DeviceStateChanged; +app/Listeners/EvaluateAutomations.php:14: public function handle(DeviceStateChanged $event): void +app/Providers/AppServiceProvider.php:5:use App\Events\DeviceStateChanged; +app/Providers/AppServiceProvider.php:19: Event::listen(DeviceStateChanged::class, EvaluateAutomations::class); +app/Events/DeviceStateChanged.php:15:class DeviceStateChanged implements ShouldBroadcastNow +app/Events/DeviceStateChanged.php:32: return 'DeviceStateChanged'; +app/Jobs/IngestShellyMessage.php:5:use App\Events\DeviceStateChanged; +app/Jobs/IngestShellyMessage.php:76: DeviceStateChanged::dispatch($device->uuid, $entity->key, $update['state']); +app/Livewire/Panel.php:18: #[On('echo-private:home,.DeviceStateChanged')] +app/Livewire/Panel.php:19: public function onDeviceStateChanged(): void {} +app/Livewire/Devices/Show.php:61: #[On('echo-private:home,.DeviceStateChanged')] +app/Livewire/Devices/Show.php:62: public function onDeviceStateChanged(): void +app/Livewire/Rooms/Show.php:22: #[On('echo-private:home,.DeviceStateChanged')] +app/Livewire/Rooms/Show.php:23: public function onDeviceStateChanged(): void {} +app/Livewire/Rooms/Index.php:13: #[On('echo-private:home,.DeviceStateChanged')] +app/Livewire/Rooms/Index.php:14: public function onDeviceStateChanged(): void {} +app/Livewire/Dashboard.php:15: #[On('echo-private:home,.DeviceStateChanged')] +app/Livewire/Dashboard.php:16: public function onDeviceStateChanged(): void +app/Livewire/Windows.php:13: #[On('echo-private:home,.DeviceStateChanged')] +app/Livewire/Windows.php:14: public function onDeviceStateChanged(): void {} +app/Models/Automation.php:14: 'conditions', 'actions', 'cooldown_seconds', 'dry_run', 'last_triggered_at', +app/Models/Automation.php:21: 'conditions' => 'array', +app/Livewire/Modals/CreateAutomation.php:82: 'conditions' => [], +database/migrations/2026_07_18_000001_create_persons_discovery_automations_tables.php:47: $table->json('conditions')->nullable(); +database/seeders/DemoHomeSeeder.php:15: * conditions (an offline device, low batteries, an open window) to exercise the warnings. +cat: 'database/migrations/*create_automations*': No such file or directory + +exec +/bin/bash -lc "sed -n '35,90p' app/Jobs/IngestShellyMessage.php; sed -n '35,70p' database/migrations/2026_07_18_000001_create_persons_discovery_automations_tables.php; cat config/queue.php | sed -n '1,80p'; grep -R \"QUEUE_CONNECTION\" -n .env.example phpunit.xml*" in /home/nexxo/homeos + succeeded in 0ms: + try { + \Illuminate\Support\Facades\Redis::connection()->incr('metrics:mqtt:count'); + } catch (\Throwable) { + // metrics are best-effort + } + + $parsed = ShellyTopics::parseStatus($this->topic); + if ($parsed === null) { + return; + } + + [$prefix, $component] = $parsed; + + // Unknown devices are handled by discovery (Phase 4), not silently created here. + $device = Device::where('config->mqtt_prefix', $prefix)->first(); + if ($device === null) { + return; + } + + $data = json_decode($this->payload, true); + if (! is_array($data)) { + return; + } + + $observedAt = $this->observedAt ?? (int) round(microtime(true) * 1_000_000); + $observedTime = \Illuminate\Support\Carbon::createFromTimestampMs(intdiv($observedAt, 1000)); + + // Presence reflects when the message was RECEIVED, and only ever advances. The guard + // lives in the WHERE clause (one atomic UPDATE), so concurrent workers can't rewind it + // via read-then-write — queue latency, retries or stale messages never mark it wrong. + Device::whereKey($device->id) + ->where(fn ($q) => $q->whereNull('last_seen_at')->orWhere('last_seen_at', '<', $observedTime)) + ->update(['last_seen_at' => $observedTime, 'updated_at' => now()]); + + foreach (ShellyNormalizer::normalize($component, $data) as $update) { + $entity = $device->entities()->firstOrCreate( + ['key' => $update['key']], + ['type' => $update['type']], + ); + + if ($this->applyState($entity->id, $update['state'], $observedAt)) { + DeviceStateChanged::dispatch($device->uuid, $entity->key, $update['state']); + } + } + } + + /** + * Monotonic, race-safe upsert. Returns true when this message's state was applied, + * false when it was stale (an equal-or-newer state is already stored). + */ + private function applyState(int $entityId, array $state, int $observedAt): bool + { + $encoded = json_encode($state); + + // Overwrite an existing row only when this message is newer. + $updated = DeviceState::query() + $table->timestamps(); + + $table->unique(['source', 'identifier']); + }); + + Schema::create('automations', function (Blueprint $table) { + $table->id(); + $table->uuid()->unique(); + $table->string('name'); + $table->boolean('enabled')->default(true); + $table->string('trigger_type'); // state_change | time + $table->json('trigger_config')->nullable(); + $table->json('conditions')->nullable(); + $table->json('actions')->nullable(); + $table->unsignedInteger('cooldown_seconds')->default(0); + $table->timestamp('last_triggered_at')->nullable(); + $table->timestamps(); + }); + } + + public function down(): void + { + Schema::dropIfExists('automations'); + Schema::dropIfExists('discovery_findings'); + Schema::dropIfExists('persons'); + } +}; + env('QUEUE_CONNECTION', 'database'), + + /* + |-------------------------------------------------------------------------- + | Queue Connections + |-------------------------------------------------------------------------- + | + | Here you may configure the connection options for every queue backend + | used by your application. An example configuration is provided for + | each backend supported by Laravel. You're also free to add more. + | + | Drivers: "sync", "database", "beanstalkd", "sqs", "redis", + | "deferred", "background", "failover", "null" + | + */ + + 'connections' => [ + + 'sync' => [ + 'driver' => 'sync', + ], + + 'database' => [ + 'driver' => 'database', + 'connection' => env('DB_QUEUE_CONNECTION'), + 'table' => env('DB_QUEUE_TABLE', 'jobs'), + 'queue' => env('DB_QUEUE', 'default'), + 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 90), + 'after_commit' => false, + ], + + 'beanstalkd' => [ + 'driver' => 'beanstalkd', + 'host' => env('BEANSTALKD_QUEUE_HOST', 'localhost'), + 'queue' => env('BEANSTALKD_QUEUE', 'default'), + 'retry_after' => (int) env('BEANSTALKD_QUEUE_RETRY_AFTER', 90), + 'block_for' => 0, + 'after_commit' => false, + ], + + 'sqs' => [ + 'driver' => 'sqs', + 'key' => env('AWS_ACCESS_KEY_ID'), + 'secret' => env('AWS_SECRET_ACCESS_KEY'), + 'prefix' => env('SQS_PREFIX', 'https://sqs.us-east-1.amazonaws.com/your-account-id'), + 'queue' => env('SQS_QUEUE', 'default'), + 'suffix' => env('SQS_SUFFIX'), + 'region' => env('AWS_DEFAULT_REGION', 'us-east-1'), + 'after_commit' => false, + ], + + 'redis' => [ + 'driver' => 'redis', + 'connection' => env('REDIS_QUEUE_CONNECTION', 'default'), + 'queue' => env('REDIS_QUEUE', 'default'), + 'retry_after' => (int) env('REDIS_QUEUE_RETRY_AFTER', 90), + 'block_for' => null, + 'after_commit' => false, + ], + + 'deferred' => [ + 'driver' => 'deferred', + ], + + 'background' => [ +.env.example:40:QUEUE_CONNECTION=redis +phpunit.xml:30: + +codex +The engine ignores persisted automation conditions and its cooldown check is race-prone with the configured queued listener. Both issues can cause device actions to execute when they should not. + +Full review comments: + +- [P1] Evaluate conditions before executing actions — /home/nexxo/homeos/app/Services/AutomationEngine.php:62-64 + Automations with non-empty `conditions` currently execute whenever their trigger matches because `maybeRun()` never inspects the condition configuration. This turns a trigger→condition→action rule into an unconditional action and can switch devices when its stored conditions are false; evaluate all configured conditions before entering the action loop. + +- [P1] Claim the cooldown atomically before running actions — /home/nexxo/homeos/app/Services/AutomationEngine.php:54-58 + When multiple queue workers process rapid state changes, each job can read the same old `last_triggered_at`, pass this check, and issue the action before either saves the new timestamp. Thus the cooldown intended to suppress event floods does not prevent duplicate commands under concurrency; reserve the run with a transaction, lock, or conditional atomic update before executing actions. +The engine ignores persisted automation conditions and its cooldown check is race-prone with the configured queued listener. Both issues can cause device actions to execute when they should not. + +Full review comments: + +- [P1] Evaluate conditions before executing actions — /home/nexxo/homeos/app/Services/AutomationEngine.php:62-64 + Automations with non-empty `conditions` currently execute whenever their trigger matches because `maybeRun()` never inspects the condition configuration. This turns a trigger→condition→action rule into an unconditional action and can switch devices when its stored conditions are false; evaluate all configured conditions before entering the action loop. + +- [P1] Claim the cooldown atomically before running actions — /home/nexxo/homeos/app/Services/AutomationEngine.php:54-58 + When multiple queue workers process rapid state changes, each job can read the same old `last_triggered_at`, pass this check, and issue the action before either saves the new timestamp. Thus the cooldown intended to suppress event floods does not prevent duplicate commands under concurrency; reserve the run with a transaction, lock, or conditional atomic update before executing actions. diff --git a/app/Services/AutomationEngine.php b/app/Services/AutomationEngine.php index e58bf0c..627e7bc 100644 --- a/app/Services/AutomationEngine.php +++ b/app/Services/AutomationEngine.php @@ -4,6 +4,7 @@ namespace App\Services; use App\Models\Automation; use App\Models\Entity; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Log; /** @@ -54,17 +55,61 @@ class AutomationEngine private function maybeRun(Automation $automation): void { - if ($automation->cooldown_seconds > 0 - && $automation->last_triggered_at !== null - && $automation->last_triggered_at->gt(now()->subSeconds($automation->cooldown_seconds))) { + // Conditions gate the actions but must not touch the cooldown clock — a rule whose + // conditions are currently false should stay armed for its next legitimate trigger. + if (! $this->conditionsMet($automation)) { + return; + } + + // Atomically claim the cooldown window: concurrent queue workers processing a burst of + // state changes would otherwise all read the same stale last_triggered_at and each fire + // the action. The conditional UPDATE lets exactly one worker win the race (H5). + if (! $this->claimCooldown($automation)) { return; } foreach ($automation->actions ?? [] as $action) { $this->runAction($automation, $action); } + } - $automation->forceFill(['last_triggered_at' => now()])->save(); + /** Every stored condition ({device, entity_key, field, equals}) must match current state (AND). */ + private function conditionsMet(Automation $automation): bool + { + foreach ($automation->conditions ?? [] as $condition) { + $entity = $this->findEntity($condition['device'] ?? null, $condition['entity_key'] ?? null); + $state = $entity?->state?->state ?? []; + $field = $condition['field'] ?? 'on'; + + if (! array_key_exists($field, $state) || $state[$field] != ($condition['equals'] ?? true)) { + return false; + } + } + + return true; + } + + /** Reserve the run atomically; returns false when the cooldown is still active. */ + private function claimCooldown(Automation $automation): bool + { + $now = Carbon::instance(now()); + + $query = Automation::query()->whereKey($automation->getKey()); + + if ($automation->cooldown_seconds > 0) { + $threshold = $now->copy()->subSeconds($automation->cooldown_seconds); + $query->where(fn ($q) => $q + ->whereNull('last_triggered_at') + ->orWhere('last_triggered_at', '<=', $threshold)); + } + + $claimed = $query->update(['last_triggered_at' => $now]) > 0; + + if ($claimed) { + $automation->last_triggered_at = $now; + } + + return $claimed; } private function runAction(Automation $automation, array $action): void @@ -72,10 +117,7 @@ class AutomationEngine $type = $action['type'] ?? null; if ($type === 'switch') { - $entity = Entity::query() - ->whereHas('device', fn ($q) => $q->where('uuid', $action['device'] ?? null)) - ->where('key', $action['entity_key'] ?? null) - ->with('device', 'state')->first(); + $entity = $this->findEntity($action['device'] ?? null, $action['entity_key'] ?? null); if ($entity === null) { return; @@ -89,7 +131,19 @@ class AutomationEngine $this->commands->setOn($entity, (bool) ($action['on'] ?? true), 'automation'); } elseif ($type === 'notify') { - Log::info("[automation".($automation->dry_run ? ' dry-run' : '')."] {$automation->name}: ".($action['message'] ?? '')); + Log::info('[automation'.($automation->dry_run ? ' dry-run' : '')."] {$automation->name}: ".($action['message'] ?? '')); } } + + private function findEntity(?string $deviceUuid, ?string $entityKey): ?Entity + { + if ($deviceUuid === null || $entityKey === null) { + return null; + } + + return Entity::query() + ->whereHas('device', fn ($q) => $q->where('uuid', $deviceUuid)) + ->where('key', $entityKey) + ->with('device', 'state')->first(); + } } diff --git a/tests/Feature/AutomationTest.php b/tests/Feature/AutomationTest.php index 86c8e5b..d6bb8b5 100644 --- a/tests/Feature/AutomationTest.php +++ b/tests/Feature/AutomationTest.php @@ -147,6 +147,78 @@ class AutomationTest extends TestCase $this->assertDatabaseHas('commands', ['source' => 'automation', 'command' => 'turn_on']); } + public function test_condition_blocks_action_when_false(): void + { + [$trigger, $triggerEntity] = $this->makeSwitch(); + [$target, $targetEntity] = $this->makeSwitch(); + // A separate sensor whose state the condition inspects. + [$sensor, $sensorEntity] = $this->makeSwitch(); + $sensorEntity->state()->update(['state' => ['on' => false]]); + + Automation::create([ + 'name' => 'Only when sensor on', + 'enabled' => true, + 'trigger_type' => 'state_change', + 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], + 'conditions' => [['device' => $sensor->uuid, 'entity_key' => $sensorEntity->key, 'field' => 'on', 'equals' => true]], + 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], + 'cooldown_seconds' => 0, + ]); + + app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); + + $this->assertDatabaseCount('commands', 0); + } + + public function test_condition_allows_action_when_true(): void + { + [$trigger, $triggerEntity] = $this->makeSwitch(); + [$target, $targetEntity] = $this->makeSwitch(); + [$sensor, $sensorEntity] = $this->makeSwitch(); + $sensorEntity->state()->update(['state' => ['on' => true]]); + + Automation::create([ + 'name' => 'Only when sensor on', + 'enabled' => true, + 'trigger_type' => 'state_change', + 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], + 'conditions' => [['device' => $sensor->uuid, 'entity_key' => $sensorEntity->key, 'field' => 'on', 'equals' => true]], + 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], + 'cooldown_seconds' => 0, + ]); + + app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); + + $this->assertDatabaseHas('commands', ['source' => 'automation', 'command' => 'turn_off']); + } + + public function test_failed_condition_does_not_start_the_cooldown(): void + { + [$trigger, $triggerEntity] = $this->makeSwitch(); + [$target, $targetEntity] = $this->makeSwitch(); + [$sensor, $sensorEntity] = $this->makeSwitch(); + $sensorEntity->state()->update(['state' => ['on' => false]]); + + $automation = Automation::create([ + 'name' => 'Armed', + 'enabled' => true, + 'trigger_type' => 'state_change', + 'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true], + 'conditions' => [['device' => $sensor->uuid, 'entity_key' => $sensorEntity->key, 'field' => 'on', 'equals' => true]], + 'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]], + 'cooldown_seconds' => 300, + ]); + + // Condition false → nothing happens and the cooldown must stay unarmed … + app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); + $this->assertNull($automation->refresh()->last_triggered_at); + + // … so once the condition becomes true, the very next trigger fires immediately. + $sensorEntity->state()->update(['state' => ['on' => true]]); + app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]); + $this->assertDatabaseCount('commands', 1); + } + public function test_disabled_automation_never_runs(): void { [$trigger, $triggerEntity] = $this->makeSwitch();