# Phase 4 Review: Automation, Scripts, Infrastructure

**Date:** 2026-06-02
**Scope:** ControlScripts layer, event routing, cron/background execution, infrastructure (config, logging, DB, CLI), Fury framework services.

---

## Critical (C)

### C1. Static script registries leak state across requests
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Lines:** 12–13
```php
protected static $regular_scripts = [];
protected static $actions_scripts = [];
```
`static` arrays accumulate scripts across all instantiations. In PHP-FPM, statics persist per worker process. Adding/removing scopes or reloading code does not clear the arrays. Tests (`ScriptsModelStateTest`) use reflection to reset, but production has no cleanup mechanism. If a scope file is deleted, its scripts remain in static memory until worker restart.

**Impact:** Stale script entries, phantom actions, memory growth, unpredictable behavior after deploys.

**Fix:** Flush statics at the start of `control_scripts_init()`, or use instance-level storage instead of static.

---

### C2. No atomic write / backup for scope file updates
**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php`
**Method:** `scope_update`
`file_put_contents($filepath, $file)` writes directly over the live PHP class file. If the request is interrupted mid-write, the file is truncated/corrupted. PHP will fail to parse it on the next request, breaking the entire scope and potentially the application.

**Impact:** Broken scope class → fatal parse error → 500 for all subsequent requests using that scope.

**Fix:** Write to a temp file, `rename()` atomically, keep the previous version as `.bak`.

---

### C3. Events post-flush handlers can hang indefinitely
**File:** `server/SHServ/Controllers/EventsController.php`
**Method:** `new_event`
After `fastcgi_finish_request()`, event handlers are dispatched. Handlers like `btn_on_online` → `sync_btn_channels` make synchronous HTTP calls to devices via `Button/Relay API`. `set_time_limit(10)` applies to the whole block, but with 5 event dispatches and multiple device calls, 10 seconds may not be enough. If a device is slow or unresponsive, the PHP process is held busy.

**Impact:** PHP-FPM worker exhaustion, delayed response for other requests, cron/scanning blocked.

**Fix:** Enforce a strict per-handler timeout (e.g. 2s), run handlers asynchronously via a queue or background job, or use non-blocking HTTP.

---

### C4. RCE via `scope_update` (reinforced)
**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php`
Any authenticated user can overwrite a scope file with arbitrary PHP. The file is immediately included on the next request via `control_scripts_init()`. Combined with no RBAC, this is trivial remote code execution.

**Impact:** Full server compromise.

**Fix:** Sandbox scope code (e.g. restricted DSL), validate with `php -l`, disable live editing in production, require admin role.

---

### C5. `sync-map.json` loaded from disk on every scope init, no caching
**File:** `server/ControlScripts/Common.php`
**Method:** `register_global_device_sync_map`
Each scope that `use`s `Common` re-reads `sync-map.json` via `file_get_contents`. With 4 scopes, the file is read 4 times per request (~9 KB each). No opcode cache or in-memory cache.

**Impact:** Unnecessary disk I/O, parse overhead on every request.

**Fix:** Load once in `App::control_scripts_init()` and pass to scopes, or cache decoded array in a static/shared variable.

---

### C6. Cron script execution stops on first failure
**File:** `server/SHServ/Controllers/CronController.php`
**Method:** `run_regular_cron_scripts`
```php
foreach($regular_scripts as $alias => $script) {
    // ...
    $script["script"]();
}
```
No `try/catch`. If one regular script throws, the exception bubbles up and the remaining scripts in the loop never execute.

**Impact:** Partial automation — some scheduled tasks silently skipped.

**Fix:** Wrap each script invocation in `try/catch`, log the error, continue the loop.

---

## High (H)

### H1. Action scripts have no error handling
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Method:** `run_action_script`
```php
$result = self::$actions_scripts[$alias]["script"]($params);
```
No `try/catch`. An exception in an action script kills the API request with a 500 and no meaningful response to the client.

**Fix:** Wrap in `try/catch`, return structured error response.

---

### H2. `get_source_code()` re-reads the source file on every script registration
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Method:** `get_source_code`
Uses `file()` + `array_slice` to extract closure source code. Called once per `add_action_script` / `add_regular_script`. For 20 scripts, the file is read 20 times.

**Impact:** Redundant disk I/O during every request bootstrap.

**Fix:** Cache file contents per class, or use `ReflectionFunction::getClosureUsedVariables` + store source once at build time.

---

### H3. `control_scripts_init()` instantiates all scopes eagerly
**File:** `server/SHServ/App.php`
**Method:** `control_scripts_init`
All `.php` files in `ControlScripts/Scopes/` are scanned, included, and instantiated on every request. There is no lazy loading — even disabled scopes run their constructors and all four `register_*` methods (though `register_actions_scripts` / `register_regular_scripts` are gated by DB state, the constructor still runs).

**Impact:** Slower bootstrap, unnecessary DB queries, memory overhead.

**Fix:** Lazy-load scopes when their scripts are actually needed, or cache registration metadata.

---

### H4. No health check / status for cron endpoints
**File:** `server/SHServ/Controllers/CronController.php`
Both `run_regular_cron_scripts()` and `status_update_scanning()` return nothing (implicit `null`). The HTTP response body is empty, HTTP status is 200 by default. There is no way to tell from the response whether scripts ran, how many succeeded, or if any failed.

**Impact:** Blind monitoring. External cron scheduler (e.g. systemd timer, crontab) cannot detect failures.

**Fix:** Return structured JSON with execution summary, error count, duration.

---

### H5. Logging writes to JSON without rotation or size limits
**File:** `server/Fury/Kernel/Logging.php`
**Method:** `dump`
Appends session logs to `Logs/d.m.Y.log.json`. File grows indefinitely. No max size, no rotation, no cleanup. On high-traffic days the file can become several GB.

**Impact:** Disk exhaustion, performance degradation on log write (lock contention, JSON re-encode of huge array).

**Fix:** Rotate by size (e.g. 10 MB), archive old logs, or switch to structured line-based logging.

---

### H6. No structured logging / severity / correlation IDs
**File:** `server/Fury/Kernel/Logging.php`
`logging()->set($place, $title, $message)` accepts only three strings. No severity levels, no request ID, no timestamps per entry (only per session), no structured fields.

**Impact:** Hard to debug production issues, impossible to trace a single request through controllers, models, and device calls.

**Fix:** Add severity, request/correlation ID, timestamp per log line, optional context array.

---

### H7. ErrorHandler may leak debug info if `debug` flag is on
**File:** `server/Fury/Modules/ErrorHandler/ErrorHandler.php`
**Method:** `view_fatal_error`
If `FCONF["debug"]` is `true`, the handler renders full file paths, source code excerpts, and stack traces as HTML. The `debug` flag is controlled by `.env` `DEBUG`. With no `.env` file, it defaults to `false`, but a misconfiguration or accidental `.env` edit exposes internals.

**Impact:** Information disclosure — file paths, code snippets, server internals.

**Fix:** In production mode, never render HTML errors; always log and return generic "Internal Server Error" JSON.

---

### H8. No DB schema / migrations
**File:** project-wide
There are no `.sql` migration files, no schema versioning, no install script. The `scripts` table definition only exists inside `tests/ScriptsModelStateTest.php` as an inline `CREATE TABLE`. Production database setup is manual and undocumented.

**Impact:** Impossible to recreate production DB, risky deploys, schema drift between environments.

**Fix:** Create `server/migrations/` with numbered SQL files and a CLI runner.

---

### H9. `.env` absent — fallbacks use insecure defaults
**File:** `server/SHServ/config.php`
If `.env` is missing, DB credentials default to `user => "root"`, `password => ""`, `host => "localhost"`. Device IP scan range defaults to `192.168.2.2–254`.

**Impact:** Accidental connection to wrong/weak DB, scan of wrong subnet.

**Fix:** Fail hard if `.env` is missing in production; do not provide insecure fallbacks.

---

### H10. `ControlScripts` constructor performs DB check per scope
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Line:** 26
```php
if($scope_folder != "ControlScripts" or (new Scripts()) -> script_state("scope", $scope_name)) {
```
For every non-ControlScripts scope, a new `Scripts` model is instantiated and a DB query is issued to check if the scope is enabled. With N scopes, N DB round-trips on every request.

**Impact:** N+1 query problem during every bootstrap.

**Fix:** Batch-fetch all scope states in a single query in `App::control_scripts_init()`.

---

### H11. `sync_map()` returns mutable reference to shared static state
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Method:** `sync_map`
Returns `self::$sync_map_storage`. Callers (scopes) can mutate the shared array, affecting other scopes.

**Impact:** Cross-scope state pollution, hard-to-debug side effects.

**Fix:** Return a deep copy, or make `sync_map_storage` immutable after init.

---

### H12. `DeviceScanner` silently discards scan failures
**File:** `server/SHServ/Tools/DeviceScanner.php`
Failed IPs are skipped without logging. There is no way to know which devices were expected but not found, or whether a device disappeared from the network.

**Impact:** Silent failures during device discovery, no audit trail for network changes.

**Fix:** Log scan results — found, lost, unreachable IPs.

---

### H13. `text-msgs.php` has empty strings for SHServ errors
**File:** `server/SHServ/text-msgs.php`
Lines 30–48: `device_not_found`, `unknown_device`, `error_of_device_auth`, `device_request_fail`, `db_error`, `action_script_not_found`, `scope_not_found`, etc. all have empty string values.

**Impact:** API returns empty `msg` fields for many errors, poor UX in clients.

**Fix:** Fill in meaningful Russian/English messages for all SHServ aliases.

---

### H14. `RequiredControlScriptsScope` mixes business logic with event handling
**File:** `server/SHServ/RequiredControlScriptsScope.php`
The `online` event handler directly mutates device entity state (`$device -> device_ip = ...; $device -> update()`). This is business logic that belongs in a service or model method, not in an event handler.

**Impact:** Business rules scattered across event handlers, hard to unit-test, duplication risk.

**Fix:** Move device online/offline state transition to `Devices` model or a dedicated service.

---

## Medium (M)

### M1. `EventsHandlers` devmode clutter
**File:** `server/SHServ/EventsHandlers.php`
All devmode handlers are registered inside an `if(FCONF["devmode"])` block at runtime. The code paths are still present and evaluated on every request.

**Fix:** Use conditional class loading or separate dev-only bootstrap file.

---

### M2. `console.php` has only one command (`get.config`)
**File:** `server/console.php`
The CLI entry point only supports dumping config (with credentials stripped). No commands for: run migrations, run cron manually, flush caches, health check, reset rate limiter, re-scan devices.

**Fix:** Add a command registry with useful ops commands.

---

### M3. `sync-map.json` has no schema validation
**File:** `server/ControlScripts/sync-map.json`
Loaded as raw JSON without validation. No checks that aliases correspond to real devices, that channel indices are within device schema, or that the graph is acyclic.

**Impact:** Invalid sync map causes silent failures (missing device → `continue` in helper) or wrong channel indices sent to devices.

**Fix:** Add a JSON Schema and a validation step at bootstrap (or at build time).

---

### M4. `ControlScriptsInterface` lacks return type declarations
**File:** `server/SHServ/Implements/ControlScriptsInterface.php`
Interface methods have no return types, while implementations use `: void`. This inconsistency weakens type safety.

**Fix:** Add `void` return type to interface methods.

---

### M5. No observability metrics for automation layer
Project-wide: no counters for events dispatched, actions executed, cron scripts run/failed, device API call latency, event handler latency.

**Fix:** Add lightweight metrics (even in-memory counters exposed via a `/metrics` endpoint) or integrate with Prometheus/OpenTelemetry.

---

### M6. `add_event_handler` discards handler return values
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Method:** `add_event_handler`
The wrapper closure ignores the return value of `$handler`. There is no way for a handler to signal "stop propagation" or return a result.

**Fix:** Return the handler result, or support explicit `stopPropagation()` mechanism.

---

### M7. `SpotlightsScope::spotlights_by_time` is an empty regular script
**File:** `server/ControlScripts/Scopes/SpotlightsScope.php`
**Line:** 64–70
Registered but does nothing. Pollutes the regular scripts list and cron loop.

**Fix:** Remove or implement the time-based logic.

---

### M8. `TestScriptsScope` references hardcoded production device
**File:** `server/ControlScripts/Scopes/TestScriptsScope.php`
**Line:** 23–26
Alias `test_stand_relay` is hardcoded. If the device does not exist, the action script will fail at runtime.

**Fix:** Gate test scopes behind `devmode` flag, or move to a separate directory excluded from production scan.

---

### M9. `EventsModel` naming inconsistency
**File:** `server/SHServ/Models/EventsModel.php`
Methods: `global_any_device_event_call`, `global_device_event_call`, `channel_device_event_call`, `alias_device_event_call`, `channel_alias_device_event_call`. Naming pattern is inconsistent (`_any_` vs `_alias_` placement).

**Fix:** Standardize naming to `{scope}_{target}_event_call`.

---

### M10. `ControlScripts` constructor uses fragile string parsing
**File:** `server/SHServ/Middleware/ControlScripts.php`
**Line:** 24
```php
list($scope_folder, $scope_name) = explode("\\", str_replace("\\Scopes", "", str_replace("SHServ", "", static::class)));
```
Assumes exactly two namespace segments. If namespace depth changes, parsing breaks.

**Fix:** Use `basename(str_replace('\\', '/', static::class))` for name, and `dirname()` logic for folder.

---

### M11. `Scripts::remove_scope` has TOCTOU race
**File:** `server/SHServ/Models/Scripts.php`
**Method:** `remove_scope`
```php
if(!file_exists($filepath)) { return false; }
if(!@unlink($filepath)) { return false; }
```
File can be modified between `file_exists` and `unlink`.

**Fix:** Just attempt `unlink` and check the result; suppress warning with `@`.

---

### M12. No request correlation ID for event dispatch chain
When a device sends an event, it triggers multiple handlers, each making device API calls. There is no tracing ID to correlate the original event with downstream actions in logs.

**Fix:** Generate a `correlation_id` in `EventsController` and pass it through all event handlers and device API calls.

---

### M13. `Logging` JSON file lacks indexing
**File:** `server/Fury/Kernel/Logging.php`
Searching logs requires loading and parsing the entire JSON file. No grep-friendly line format.

**Fix:** Use newline-delimited JSON (NDJSON) for append-only, grep-friendly log files.

---

### M14. `ControlScripts` constructor bypass for `RequiredControlScriptsScope`
**File:** `server/SHServ/App.php`
`RequiredControlScriptsScope` is instantiated directly before scanning the `Scopes/` directory. It is not listed in `control_scripts_instances` (it is in `$required_control_scripts_instance`). But its scripts are still registered in the same static arrays, mixing required and optional scopes.

**Impact:** Required scope handlers can be accidentally disabled if static arrays are cleared.

**Fix:** Separate static storage for required vs optional scopes, or ensure required scope is always registered regardless of DB state.

---

### M15. `error_handler` in config only covers warnings and fatal errors
**File:** `server/SHServ/config.php`
```php
"important_errors" => ["E_WARNING", "E_ERROR", "E_CORE_ERROR", "EXCEPTION"]
```
Notices, deprecations, and parse errors are silently ignored in production. This can hide bugs.

**Fix:** Include `E_NOTICE`, `E_DEPRECATED`, `E_PARSE` in dev; log (but don't display) them in production.

---

## Summary

| Severity | Count |
|----------|-------|
| Critical | 6 |
| High     | 14 |
| Medium   | 15 |

**Key themes:**
1. **Static state leakage** — `ControlScripts` static arrays cause cross-request and cross-test contamination.
2. **Missing error boundaries** — Cron scripts, action scripts, and event handlers run without `try/catch`, causing cascading failures.
3. **Unsafe file I/O** — Scope updates are non-atomic; sync-map is re-read redundantly.
4. **No observability** — No metrics, no tracing, no structured logging, empty error messages.
5. **Infrastructure gaps** — No DB migrations, minimal CLI, no log rotation, `.env` fallbacks are insecure.