Claude finished @Yeraze's task —— View job

PR Review

This is a well-structured PR with clear separation of concerns. Here's a comprehensive review:

Security

✅ PSK never exposed to the browser — GET /api/automations/channels (automationRoutes.ts:68) passes the raw psk into unifyChannels(), but unifyChannels only calls getEncryptionStatus(ch.psk) (a one-way fingerprint check) and never includes the PSK in the returned UnifiedChannel type. The channelUnify.test.ts:63 "does not expose the raw psk" assertion confirms this.

⚠️ action.notify URL injection is partially mitigated but reviewable (actionExecutor.ts:132):

const rawUrls = typeof p.urls === 'string'
  ? await interpolateAsync(p.urls, ctx, { varsOnly: true })
  : '';

The { varsOnly: true } flag restricts interpolation to {{ var.* }} tokens so {{ trigger.text }} (mesh-controlled input) can't inject an Apprise URL. However, variable values themselves could still be attacker-influenced if a node can write to a writable variable via automation. This is an acceptable design given the permission model, but worth noting in docs.

✅ Prototype pollution guarded in graphEvaluator.ts:110 — condition results use a Map, and the final Object.fromEntries is safe (documented in comment at line 194).

✅ ReDoS protected — both messageMatchesFilter and conditionEvaluator.ts use compileUserRegex (RE2 / linear-time).

Potential Bugs

🐛 geofenceState map grows unbounded (automationEngineService.ts:79):

private geofenceState = new Map<string, boolean>();

One entry per automationId:nodeNum pair is added on every checkGeofences call and never evicted. In a long-running server with many nodes tracked across multiple geofence automations, this will grow indefinitely. Consider capping or TTL-expiring entries — e.g., evict when the automation is reloaded or after N days without a position update.

🐛 Geofence state not reset on load() — when automations are reloaded (load() at line 91), this.geofenceState is not cleared. If a geofence automation is deleted and a new one created with the same ID (unlikely, but possible if UUIDs are reused), stale state carries over. Clearing on load() would be safer:

this.geofenceState.clear(); // add after: this.index = index;

🐛 action.sendMessage with multi-source + DM falls back to trigger's source (actionExecutor.ts:83-86):

if (destination != null || channelSel.length === 0) {
  results.push(await deps.sendMessage({ sourceId: sid, text, channel: fallbackChannel, destination, replyId }));

When sourceIds is multi-select AND destination != null (DM), the action sends a DM on every selected source. This may be intentional, but a user selecting 3 sources + DM target could inadvertently send 3 DMs. The UI doesn't warn about this.

🐛 Empty channel name display in the builder (AutomationBuilder.tsx:152):

{' '}{c.name || '(Primary)'}

This correctly shows (Primary) for the primary Meshtastic channel (empty name), but if a MeshCore channel has an empty name the same label appears — even though "Primary" is a Meshtastic concept. Minor cosmetic issue.

🐛 formatTimestamp uses local time of the server process (interpolate.ts:29):

return `${d.getFullYear()}-${p(d.getMonth() + 1)}-${p(d.getDate())} ...`;

new Date(ms) formats in the server's local timezone, not the user's. If the server runs in UTC but the operator is in UTC-5, timestamps in messages will be wrong. Consider using UTC methods or documenting that the timestamp is server-local.

Code Quality

✅ Clean architecture — ActionDeps / NodeDataProvider interfaces properly decouple IO from logic; the simulator wraps both cleanly without exposing live systems.

✅ Back-compat — normalizeGeofenceParams (geo.ts:65) correctly prefers the new structured shape param but falls back to legacy flat lat/lon/radiusKm.

⚠️ Duplicated simApplySetVar logic (automationSimulator.ts:171): This function is a near-exact copy of AutomationEngineService.applySetVar (automationEngineService.ts:135). If the setVar semantics change (new op, different delta logic), both need updating. Consider extracting to a shared helper, or making applySetVar a standalone function the engine class calls.

⚠️ channelUnify.ts is a 0-byte stub in the diff — the SHA listed in the changed files is for an empty file (6dfea73...), yet the file clearly has content when read. This is likely a diff artifact, but worth verifying the committed file is non-empty.

⚠️ GeofenceFieldInput.tsx radio button name collision (GeofenceFieldInput.tsx:29):

<input type="radio" name="ae-geofence-shape" ...

If multiple GeofenceFieldInput instances are rendered on the same page (e.g., in a FINALLY step), they share the same name attribute and the radio buttons will interfere with each other. Use a unique id (e.g., useId() hook or a random suffix).

⚠️ isMsTimestampPath regex is broad (interpolate.ts:22):

return path === 'NOW' || /(^|\.)timestamp$/i.test(path);

Any path ending in timestamp (e.g. trigger.timestamp, var.myTimestamp) is formatted as a human-readable date. This is good for trigger.timestamp, but if a user has a variable named lastTimestamp whose value is a Unix timestamp in seconds (not ms), it will format as a date circa year 52000. The var.* namespace probably shouldn't be caught by this heuristic.

Performance

✅ Lazy channel-name DB lookup (automationEngineService.ts:216): The channelName DB lookup is skipped unless a loaded automation actually uses channelName filtering — good hot-path optimization.

⚠️ Per-send getSourceProtocol + getChannels DB calls (actionExecutor.ts:88-89): For each source in a multi-source send, two DB queries are issued in the action execution path:

const proto = (await ctx.data.getSourceProtocol?.(sid)) ?? null;
const srcChannels = (await ctx.data.getChannels?.(sid)) ?? [];

These are called inside the for (const sid of sourceIds) loop. For a 5-source × 10-channel matrix, that's 10 DB queries per action execution. Caching these per-run in the EngineEvalContext or batching them before the loop would help for high-throughput deployments.

Test Coverage

✅ Strong new test coverage: geo.test.ts, channelUnify.test.ts, and additions to actionExecutor.test.ts, automationEngineService.test.ts, automationSimulator.test.ts all cover the new code paths well.

⚠️ Missing test: unbounded geofenceState growth — no test asserts the map size or tests reload behavior.

⚠️ Missing test: multi-source × DM send semantics — the behavior of sending a DM to multiple sources is not explicitly tested.

⚠️ Missing test: formatTimestamp on a seconds-epoch value — no test covers the var.someTimestamp heuristic mis-formatting a seconds value.

Minor / Nits

• catalog.ts — the trigger.schedule entry should have a comment or label clearly indicating it's a stub/not-yet-firing, so future contributors don't assume it's live.
• automationRoutes.ts:110: error: any type assertion is acceptable here but could be narrowed with an instanceof Error guard.
• The ← Dashboard BASE_URL fix in AutomationsPage.tsx is a good correctness fix; no issues there.

Summary

This is a high-quality, well-tested PR with thoughtful security design (PSK non-exposure, ReDoS protection, prototype pollution guard). The main actionable items before merge:

1. Bug: geofenceState map grows unbounded — add eviction or cap.
2. Bug: geofenceState not cleared on load() — minor correctness risk.
3. Bug: formatTimestamp uses server local timezone — document or fix to UTC.
4. Design: Duplicated applySetVar logic between engine and simulator.
5. Performance: getSourceProtocol + getChannels called inside send loop — consider caching per-run.
6. UX: GeofenceFieldInput radio name collision if rendered twice.

The trigger.schedule live-firing stub is explicitly called out in the PR description as a follow-up, which is appropriate — the test-panel dry-run path is solid.

Claude finished @Yeraze's task —— View job

PR Review

• Read changed files
• Analyze security concerns
• Analyze potential bugs
• Analyze code quality
• Analyze performance
• Analyze test coverage
• Post final review

This is a large, well-structured PR adding real capability to the Automation Engine. The code quality is consistently high and the PR description is honest about what's stubbed vs. shipped. Here's my detailed review:

Security

✅ PSK never sent to the browser — GET /api/automations/channels (automationRoutes.ts:76) maps raw channel rows (including psk) into unifyChannels(), which only calls getEncryptionStatus(ch.psk) (a one-way fingerprint). The channelUnify.test.ts:63 "does not expose the raw psk" assertion directly covers this.

✅ URL injection protected (actionExecutor.ts:132) — { varsOnly: true } prevents mesh-controlled {{ trigger.* }} tokens from being interpolated into Apprise urls, which could otherwise redirect notifications to attacker-controlled endpoints.

✅ ReDoS protected — conditionEvaluator.ts:43 uses compileUserRegex (RE2 / linear-time) for user-supplied regex patterns.

✅ Prototype pollution guarded (graphEvaluator.ts:110-111) — condition results accumulate in a Map, and the comment at line 194 explicitly calls out why Object.fromEntries is safe against __proto__ node IDs.

⚠️ action.notify URL injection residual risk — variable values could themselves be attacker-influenced if automation flow.setVar writes from mesh-controlled context. The permission model mitigates this (only admins create automations), but it's worth documenting.

Bugs

🐛 geofenceState grows unbounded (automationEngineService.ts:79):

private geofenceState = new Map<string, boolean>();

One entry per ${automationId}:${nodeNum} pair is written on every checkGeofences call and never evicted. A deployment tracking 1000 nodes across 10 geofence automations accumulates 10,000 entries permanently. Consider evicting on automation reload, or using a timestamp-based TTL.

🐛 geofenceState not cleared on load() (automationEngineService.ts:91):
When automations are reloaded, the geofence state map is not cleared. While UUID collisions on re-creation are unlikely, it's still safer to this.geofenceState.clear() after this.index = index on line 117.

🐛 Multi-source DM sends to every selected source (actionExecutor.ts:83-85):

if (destination != null || channelSel.length === 0) {
  results.push(await deps.sendMessage({ sourceId: sid, text, channel: fallbackChannel, destination, replyId }));

When a user selects 3 sources AND a DM destination, the DM is sent on all 3 sources. The UI doesn't warn about this — a user selecting "all my radios" for broadcast channel sends may inadvertently 3x a DM.

🐛 (Primary) label applied to all nameless MeshCore channels (AutomationBuilder.tsx:152):

{' '}{c.name || '(Primary)'}

MeshCore channels with empty names are shown as (Primary), but that's a Meshtastic concept. Minor cosmetic issue, but could confuse users.

🐛 formatTimestamp uses server's local timezone (interpolate.ts:31):

return `${d.getFullYear()}-${p(d.getMonth() + 1)}-${p(d.getDate())} ${p(d.getHours())}:${p(d.getMinutes())}:${p(d.getSeconds())}`;

d.getHours() etc. use the server process's local time, not the user's timezone or UTC. A UTC server with a UTC-5 operator produces timestamps 5 hours off. The simplest fix is d.getUTCFullYear() etc. and appending UTC; or document this as server-local.

🐛 condition.timeRange uses server local time (conditionEvaluator.ts:128) — same issue: new Date(ctx.now) then getHours()/getDay() are server-local. If users intend local time but the server is in a different timezone, the time-range condition won't fire when expected. (Not new in this PR, but worth noting given the new condition.always addition exposes the evaluator to more scrutiny.)

Code Quality

✅ Clean architecture — ActionDeps / NodeDataProvider interfaces properly decouple IO from logic. The recordingDeps() in the simulator wraps both cleanly.

✅ Back-compat — normalizeGeofenceParams (geo.ts:65) correctly prefers the new shape param but falls back to legacy flat lat/lon/radiusKm, so saved automations keep working.

✅ useId() for radio name uniqueness — GeofenceFieldInput.tsx:19 uses useId() to avoid radio button interference when multiple editors render on one page. The earlier review noted this as a risk; it's already been addressed.

⚠️ Duplicated applySetVar logic (automationSimulator.ts:171 vs automationEngineService.ts:135):
simApplySetVar is a near-exact copy of AutomationEngineService.applySetVar. If a new variable op is added (e.g. append), both need to be updated. The engine method is private, but extracting it to a shared standalone function (e.g. applySetVarOp(node, ctx, vars)) would eliminate the duplication while keeping both callers testable.

⚠️ isMsTimestampPath heuristic has edge cases (interpolate.ts:23-25):

function isMsTimestampPath(path: string): boolean {
  return path === 'NOW' || path === 'trigger.timestamp';
}

The current implementation is correctly narrowed to only NOW and trigger.timestamp (good — the comment documents why var.* is excluded), but there's no guard against a user naming their variable NOW and getting an accidental timestamp formatting of its value. This is a very minor edge case given the value would need to be in ms-epoch range to look wrong.

⚠️ catalog.ts should mark trigger.schedule as a stub — the schedule entry should have a visible indicator (e.g. label suffix (coming soon) or a stub: true field) so contributors don't assume it fires. The PR description mentions this in the follow-ups, but a code-level marker is more durable.

✅ condition.always is clean (conditionEvaluator.ts:71-72) — simple, explicit, and well-named. The FINALLY ALWAYS mode (graphEvaluator.ts:132-133) correctly activates unconditionally regardless of rule outcomes.

Performance

⚠️ Per-send DB queries inside loop (actionExecutor.ts:88-89):

const proto = (await ctx.data.getSourceProtocol?.(sid)) ?? null;
const srcChannels = (await ctx.data.getChannels?.(sid)) ?? [];

These two DB calls happen inside the for (const sid of sourceIds) loop. For N sources × M selected channels, that's 2N DB calls per action execution. The overhead is small for typical deployments (1-5 sources), but batching (e.g., fetching all needed sources' protocol + channels before the loop) would scale better for users with many sources.

✅ Lazy channel-name DB lookup (automationEngineService.ts:216-222) — the channelName lookup is gated behind usesChannelName, keeping the hot path DB-free for automations that don't use name-based filtering.

Test Coverage

✅ Strong new coverage: geo.test.ts (circle, polygon, edge cases), channelUnify.test.ts (PSK non-exposure, protocol separation, disabled slots), actionExecutor.test.ts (multi-source matrix, back-compat), automationEngineService.test.ts (geofence state), and automationSimulator.test.ts (schedule dry-run, source-filter, condition.always, action.nothing).

⚠️ Missing: geofenceState eviction/size test — no test asserts map growth behavior or that load() affects state.

⚠️ Missing: multi-source × DM send semantics test — the case where destination != null AND sourceIds has multiple entries is not explicitly tested.

⚠️ Missing: formatTimestamp timezone test — no test exercises what happens with a very large ms value or documents the server-local behavior.

Minor / Nits

• automationRoutes.ts:110: error: any is fine here, but error instanceof Error ? error.message : String(error) would be slightly more TypeScript-idiomatic.
• The ← Dashboard BASE_URL fix in AutomationsPage.tsx is a straightforward correctness fix with no issues.
• The substitutions help panel moving to a non-modal slide-in (SubstitutionsHelpDrawer) is a UX improvement — no code issues.
• AutomationTester.tsx:273 shows channel ${p.channel ?? 0} for a multi-source send — if the resolved params contains an array of results, this won't render usefully. Could be improved, but test panel display is non-critical.

Summary

Ship-blocking: None, given the PR explicitly calls out trigger.schedule as a stub.

Recommended before merge:

1. geofenceState unbounded growth — low urgency for now, but add a this.geofenceState.clear() in load() at minimum to handle the reload case cleanly.
2. Document formatTimestamp server-local behavior — either switch to UTC methods or add a comment/doc note.
3. Extract shared applySetVarOp — prevents future drift between engine and simulator.

Fine as follow-ups (per PR description):

• Schedule trigger live cron firing
• {{ }} token smart entry

The test suite is comprehensive, the security design is sound, and the feature set is well-scoped.
| Branch: worktree-automation-geofence-map

Thanks for the review. Triage of the flagged items (addressed in 8b9f4c5f):

Fixed

• 🐛 isMsTimestampPath too broad → narrowed to exactly {{ NOW }} and {{ trigger.timestamp }}, so a {{ var.*timestamp }} (unknown units) is never mis-formatted. Added a test.
• ⚠️ GeofenceFieldInput radio name collision → now uses useId().
• nit: trigger.schedule stub → added a code comment that it isn't fired live yet (also tracked in AUTOMATION_ENGINE_PLAN.md §11).

Not an issue

• "channelUnify.ts is a 0-byte stub" → diff artifact; the committed file is 2778 bytes (verified git show HEAD:…).
• PSK non-exposure / ReDoS / prototype-pollution → confirmed correct, thanks.

Pre-existing (Phase 1a 789d269f, not introduced by this PR) — deferring to a follow-up, not blocking this one:

• geofenceState unbounded growth + not cleared on load() — real but pre-existing engine lifecycle behavior.
• Duplicated simApplySetVar vs applySetVar.

Judgment calls, intentionally left as-is:

• formatTimestamp server-local time — intentional (the server is the operator's) and documented as "local date/time" in the substitutions help; UTC would be more surprising for most single-operator deployments.
• Multi-source + DM sends one DM per selected source — that's the intended "matrix" semantics (the user chose those sources); a UI guard can come later.
• Per-source getSourceProtocol/getChannels in the send loop — these are 2 queries per source (not per channel; getChannels is hoisted above the channel loop). Fine for typical fan-out; per-run caching is a reasonable future optimization.