Claude finished @Yeraze's task —— View job

PR Review: feat(automations): "Run a script" action + JSON result variables

Tasks:

• Read PR context and understand scope
• Review backend implementation (actionExecutor.ts, engineContext.ts, variableCodec.ts)
• Review frontend changes (AutomationBuilder.tsx, catalog.ts)
• Review test coverage
• Post comprehensive review feedback

Summary

This is a well-structured PR that completes the Automation Engine's "Run a script" action. The implementation is clean, well-tested, and follows existing project conventions. Below are observations across the review areas.

Code Quality & Best Practices

Strengths:

• The injectable ActionDeps.runScript interface (actionExecutor.ts:32-33) cleanly separates script execution from the action logic, maintaining the existing testability pattern.
• triggerEnv() (actionExecutor.ts:43-66) is a pure function, making it independently testable. The MM_<UPPER_SNAKE> key derivation regex ($1_$2) handles camelCase correctly.
• resolveVarValue() in engineContext.ts:97-112 is a clean, shared helper for nested JSON path traversal used by both interpolation and condition evaluation — no duplication.
• The simulator stub (automationSimulator.ts:118-120) correctly prevents dry-runs from spawning processes.
• The variable name dot-free validation pattern (/^[A-Za-z_][A-Za-z0-9_]*$/, AutomationsPage.tsx:308) ensures the var.name.path split remains unambiguous.

Minor Issues:

1. variableCodec.ts:66 — "JSON string stored as-is" is a subtle trap. When a user stores the string "hello" into a json variable, encodeValue stores hello (no wrapping), but decodeValue then calls JSON.parse('hello') and returns null because bare words are not valid JSON. The test covers round-tripping '{"a":1}' but not plain string inputs like 'hello'. Consider whether typeof value === 'string' should also wrap the string as JSON (i.e. JSON.stringify(value)) for consistent round-tripping of all string inputs.

2. actionExecutor.ts:107 — Implicit fallback from JSON to raw stdout. When result.returnValue is undefined (non-JSON script output), the action stores result.stdout.trim() as a string into the result variable. This will be stored into a json-typed variable as a plain string, which decodeValue will then fail to JSON.parse, returning null. The behavior is not wrong, but users may be surprised that a non-JSON script with a result variable configured silently stores null instead of the raw string. Worth a log warning or at minimum documenting in the UI help text.

3. conditionEvaluator.ts:94-98 — condition.variable with dot-notation names. The condition accepts variable: 'obj.stats.count' (as tested at conditionEvaluator.test.ts:124), but the validation in automation.ts:316-318 only validates p.variable for the plain-name case — it doesn't block dots. This is intentional (dots enable JSON path access), but it means the variable name constraint documented in the UI (no dots) is asymmetric with what's actually enforced on the backend for condition.variable. Might be worth a brief comment near the validation to note this.

Potential Bugs

4. engineContext.ts:105 — Empty-segment filter in path resolution. fullName.slice(dot + 1).split('.').filter(Boolean) correctly handles trailing dots, but it also silently swallows double-dots (e.g., obj..field → treats as obj.field). This is cosmetic but could confuse users who mistype a path. Not a security issue since the traversal is against in-memory objects, not the filesystem.

5. actionExecutor.ts:99-100 — timeoutSeconds minimum of 1. Math.max(1, Number(p.timeoutSeconds)) coerces any value ≤ 0 to 1 second, including the user entering 0 meaning "no timeout". The existing scriptRunner.ts default is 30s when timeoutMs is undefined. The action passes undefined when timeoutSeconds is not set, correctly deferring to the 30s default. But a user setting timeoutSeconds: 0 to try to "reset to default" will instead get a 1s timeout. The UI placeholder says 30, which is consistent with the default, but the zero-case coercion is a footgun. Consider treating 0 as "use default" by checking p.timeoutSeconds != null && Number(p.timeoutSeconds) > 0.

Performance Considerations

6. engineContext.ts:130 — Serial await in interpolateAsync. The loop at line 130-138 awaits each path resolution sequentially. For a template with multiple {{ var.* }} references (e.g., {{ var.a }}, {{ var.b }}, {{ var.c }}), these DB lookups run serially. Given that variable lookups hit SQLite (which is synchronous underneath), this is fine in practice, but a parallel Promise.all over paths + resolvePath would be a trivial improvement if performance becomes a concern with many variables.

Security Considerations

The security story here is solid:

• Path traversal protection is handled in the existing resolveScriptPath() (scriptRunner.ts:104-128) with path.resolve + prefix check + extension allowlist. The new action delegates to this — no new attack surface.
• Environment variable injection is safe: triggerEnv() maps fields to MM_* keys, and the values are either String() coerced or JSON.stringify()'d — no shell interpolation since execFile (not exec) is used.
• varsOnly security guard for Apprise URLs (actionExecutor.ts:184) is correctly unrelated to runScript — the new action doesn't inherit this concern.
• The simulator stub prevents spawning during dry-runs — important since the test UI calls simulate on untrusted configs.

One observation:

7. triggerEnv() exposes all trigger fields as env vars including any user-controlled string content (e.g. MESSAGE, MM_TEXT). This is the intended design (scripts are meant to act on message content), but it means a script receiving MM_TEXT='; rm -rf /' needs to handle that itself. This is not a bug in MeshMonitor — it's inherent to user scripts — but worth a note in user documentation that scripts must sanitize inputs.

Test Coverage

Coverage is good overall:

• triggerEnv mapping — ✅ actionExecutor.test.ts:223-233
• runScript dep call + result var write — ✅ actionExecutor.test.ts:235-246
• runScript failure throws — ✅ actionExecutor.test.ts:248-253
• scriptPath required — ✅ actionExecutor.test.ts:255-258
• json codec round-trip + malformed→null — ✅ variableCodec.test.ts:70-84
• nested {{ var.x.y.z }} interpolation — ✅ conditionEvaluator.test.ts:113-126
• simulator records runScript without spawning — ✅ automationSimulator.test.ts

Gaps worth considering:

8. No test for the stdout-fallback path (actionExecutor.ts:107): when returnValue is undefined (non-JSON script output) and resultVariable is set. The recorder stub always returns { ok: 1 } as returnValue, so the fallback branch is untested.

9. No test for timeoutSeconds edge cases (0, negative, non-numeric string) in actionExecutor.ts:99-100. Currently the Math.max(1, ...) behavior is implicit.

Summary

This is a well-implemented feature. The core concerns are:

• The json codec's "string stored as-is" behavior ("Claude PR Assistant workflow" #1) is the most likely source of user confusion.
• The timeoutSeconds: 0 footgun (feat: add telemetry graphs, node sorting, and fix duplication #5) is minor but easy to fix.
• The simulator stub and varsOnly security guard are correctly in place.

None of the above are blockers — the PR is in good shape to merge once the author has reviewed these points.