🔴 Background subshell variable assignments are invisible to parent shell, making all gathered data empty

The parallel execution pattern KLAVIYO_DATA=$(gather_klaviyo) & runs the command substitution and assignment inside a background subshell. Variable assignments in subshells do not propagate back to the parent shell. After wait, KLAVIYO_DATA, META_DATA, GA4_DATA, and GSC_DATA are all empty/unset in the parent. With set -u (line 5), the script will immediately fail with an "unbound variable" error at line 179 when trying to use $KLAVIYO_DATA. Compare with the working pattern in claude-ops/bin/ops-gather which correctly writes subshell output to temp files and reads them back.

The parallel execution pattern on lines 171-174 assigns command substitution results in background subshells, but those assignments never propagate to the parent shell. After wait, all four variables (KLAVIYO_DATA, META_DATA, GA4_DATA, GSC_DATA) are empty/unset, causing the script to crash at line 179 due to set -u.

The existing ops-gather script (claude-ops/bin/ops-gather) demonstrates the correct pattern: create a temp directory, run each function writing to a temp file in the background, wait, then read the files back. Apply the same pattern here:

1. Create a temp directory with mktemp -d and a trap to clean it up
2. Change each background call to: gather_klaviyo > "$TMPDIR/klaviyo.json" &
3. After wait, read the files: KLAVIYO_DATA=$(cat "$TMPDIR/klaviyo.json")
4. Then use the variables in the final jq command as before

Was this helpful? React with 👍 or 👎 to provide feedback.

🔴 Keyword email is ambiguous — matches both comms and marketing routes

The keyword email now appears in two routing table entries: line 24 routes it to /ops-comms $ARGUMENTS and line 33 routes it to /ops:ops-marketing $ARGUMENTS. When a user types email as the argument, the router has conflicting instructions and may pick either destination unpredictably, breaking the expected behavior for one or both features.

Was this helpful? React with 👍 or 👎 to provide feedback.

🟡 Shell injection via GSC_SITE in python3 single-quoted string

On line 143, GSC_SITE is interpolated directly into a Python single-quoted string literal: print(urllib.parse.quote('${GSC_SITE}', safe='')). If GSC_SITE contains a single quote (e.g., a user mistakenly configures it as https://example.com'; import os; os.system('...')), the shell expansion breaks out of the Python string and executes arbitrary code. While the value is user-configured (self-inflicted), this is still a code injection vector that should be sanitized, e.g., by passing the value as a command-line argument to python3 rather than interpolating it into the source code.

Was this helpful? React with 👍 or 👎 to provide feedback.

Bug: Assigning variables from backgrounded commands like VAR=$(command) & will result in empty variables, as the assignment happens in a subshell. This causes a downstream jq failure.
_{Severity: CRITICAL}

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: claude-ops/bin/ops-marketing-dash#L171-L175

Potential issue: The script executes data gathering functions like `gather_klaviyo` in
the background and attempts to assign their output to variables (e.g.,
`KLAVIYO_DATA=$(gather_klaviyo) &`). Because these assignments run in a background
subshell, the variables (`KLAVIYO_DATA`, `META_DATA`, etc.) remain empty in the parent
shell. These empty strings are then passed to `jq` via the `--argjson` flag. Since an
empty string is not valid JSON, `jq` will exit with an error, causing the entire
`/ops:marketing` dashboard script to fail deterministically on every execution.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

Capture parallel gather output in parent shell

These backgrounded assignments (KLAVIYO_DATA=$(...) & etc.) execute in subshells, so the parent shell never receives the variable values; with set -u, the later jq --argjson ... "$KLAVIYO_DATA" expansion aborts with an unbound-variable error and the dashboard fails before emitting JSON. Running bash claude-ops/bin/ops-marketing-dash currently exits on this path, so results need to be collected via temp files/FIFOs (or another parent-shell-safe pattern) instead of backgrounded assignments.

Useful? React with 👍 / 👎.

Remove duplicate email alias from ops router

Adding email to this marketing route makes routing ambiguous because email is already mapped to /ops-comms earlier in the same table, so /ops email no longer has a deterministic target and can keep dispatching to comms instead of marketing. Since this file is the router instruction source, duplicate aliases make the new marketing entry unreliable; use non-overlapping keywords or remove one mapping.

Useful? React with 👍 / 👎.

⚠️ Potential issue | 🟡 Minor

Potential command injection in URL encoding.

The GSC_SITE variable is interpolated directly into a Python string. If it contains a single quote, it could break out of the string context.

-  GSC_SITE_ENCODED=$(python3 -c "import urllib.parse; print(urllib.parse.quote('${GSC_SITE}', safe=''))" 2>/dev/null \
-    || echo "${GSC_SITE}" | sed 's|:|%3A|g; s|/|%2F|g')
+  GSC_SITE_ENCODED=$(GSC_SITE="$GSC_SITE" python3 -c "import urllib.parse, os; print(urllib.parse.quote(os.environ['GSC_SITE'], safe=''))" 2>/dev/null \
+    || printf '%s' "${GSC_SITE}" | sed 's|:|%3A|g; s|/|%2F|g')

Verify each finding against the current code and only fix it if needed.

In `@claude-ops/bin/ops-marketing-dash` around lines 143 - 144, The current
command injects GSC_SITE directly into a Python string in the python3 -c
invocation (producing GSC_SITE_ENCODED), which allows a value containing a
single quote to break out; change the invocation to pass GSC_SITE via the
environment or stdin so it is not interpolated into the source string (e.g.,
read os.environ['GSC_SITE'] or read from stdin inside the Python snippet) and
fall back to the sed encoding only if the python invocation fails; update
references to GSC_SITE_ENCODED and the python3 -c usage accordingly so input is
safely obtained from the environment/STDIN rather than string interpolation.

⚠️ Potential issue | 🔴 Critical

Critical: Background subshell variable assignments won't propagate to parent shell.

When you run VAR=$(command) &, the variable assignment occurs in a subshell. After wait, the parent shell's variables (KLAVIYO_DATA, META_DATA, etc.) remain unset/empty, causing the final jq command on lines 178-184 to fail.

-# Run all in parallel (subshells)
-KLAVIYO_DATA=$(gather_klaviyo) &
-META_DATA=$(gather_meta) &
-GA4_DATA=$(gather_ga4) &
-GSC_DATA=$(gather_gsc) &
-wait
-
-# Emit unified JSON
-jq -n \
-  --argjson klaviyo "$KLAVIYO_DATA" \
-  --argjson meta "$META_DATA" \
-  --argjson ga4 "$GA4_DATA" \
-  --argjson gsc "$GSC_DATA" \
-  --arg date "$TODAY" \
-  '{date: $date, klaviyo: $klaviyo, meta: $meta, ga4: $ga4, gsc: $gsc}'
+# Run all in parallel using temp files
+TMPDIR=$(mktemp -d)
+trap 'rm -rf "$TMPDIR"' EXIT
+
+gather_klaviyo > "$TMPDIR/klaviyo.json" &
+gather_meta > "$TMPDIR/meta.json" &
+gather_ga4 > "$TMPDIR/ga4.json" &
+gather_gsc > "$TMPDIR/gsc.json" &
+wait
+
+# Emit unified JSON
+jq -n \
+  --slurpfile klaviyo "$TMPDIR/klaviyo.json" \
+  --slurpfile meta "$TMPDIR/meta.json" \
+  --slurpfile ga4 "$TMPDIR/ga4.json" \
+  --slurpfile gsc "$TMPDIR/gsc.json" \
+  --arg date "$TODAY" \
+  '{date: $date, klaviyo: $klaviyo[0], meta: $meta[0], ga4: $ga4[0], gsc: $gsc[0]}'

Verify each finding against the current code and only fix it if needed.

In `@claude-ops/bin/ops-marketing-dash` around lines 170 - 184, The backgrounded
assignments like KLAVIYO_DATA=$(gather_klaviyo) & run in subshells so the parent
shell never gets the results; change the pattern to have each gather_* producer
write its JSON to a unique temporary file (e.g., tmp_klaviyo, tmp_meta, tmp_ga4,
tmp_gsc) while still running in background (call gather_klaviyo > "$TMP_KLAVIYO"
& etc.), wait for all to finish, then read those files into KLAVIYO_DATA,
META_DATA, GA4_DATA, GSC_DATA (or pass them to jq via --argfile/--slurpfile) and
clean up the temp files; use the gather_klaviyo, gather_meta, gather_ga4,
gather_gsc names to locate where to redirect output.

Background subshell variable assignments are silently lost

High Severity

VAR=$(func) & runs the assignment in a background subshell, so the parent shell never receives the values. After wait, KLAVIYO_DATA, META_DATA, GA4_DATA, and GSC_DATA are all empty, causing the final jq --argjson to fail on invalid (empty) JSON input. The existing ops-gather script in the same repo correctly solves this by redirecting output to temp files and reading them after wait.

 

^{Reviewed by Cursor Bugbot for commit d6206d7. Configure here.}

Keyword "email" routes to two conflicting skills

Medium Severity

The keyword email appears in both the comms route (line 24, dispatching to /ops-comms) and the new marketing route (line 33, dispatching to /ops:ops-marketing). This ambiguity means /ops email could be routed to either skill depending on which row the router matches first, leading to unpredictable behavior.

 

^{Reviewed by Cursor Bugbot for commit d6206d7. Configure here.}

Meta account ID Doppler fallback incorrectly gated

Medium Severity

The Doppler fallback for META_ACCOUNT is nested inside the if [ -z "$META_TOKEN" ] block. If META_TOKEN is resolved via env var or plugin config but META_ACCOUNT is not, the Doppler lookup for META_AD_ACCOUNT_ID is skipped entirely. This causes gather_meta to silently return null even when the account ID exists in Doppler.

 

^{Reviewed by Cursor Bugbot for commit d6206d7. Configure here.}

🟡 META_AD_ACCOUNT_ID Doppler fallback only runs when META_ADS_TOKEN is also missing

On lines 18-21, the Doppler lookup for META_AD_ACCOUNT_ID is nested inside the if [ -z "$META_TOKEN" ] block. This means if a user has META_ADS_TOKEN set via environment variable or plugin config but has META_AD_ACCOUNT_ID only in Doppler, the account ID will never be resolved. The gather_meta() function (line 56) will then return null because META_ACCOUNT is empty, silently skipping all Meta Ads data even though valid credentials exist.

Was this helpful? React with 👍 or 👎 to provide feedback.

Resolve Shopify creds from plugin config in health script

ops-ecom-health only reads SHOPIFY_STORE_URL and SHOPIFY_ACCESS_TOKEN, so users who configure the newly added plugin settings (shopify_store_url and shopify_admin_token) still hit missing_credentials. This breaks the documented /ops:ops-ecom setup path unless users also duplicate the values into env vars or Doppler, which makes the health check fail in a common configured state.

Useful? React with 👍 / 👎.

Preserve Meta account when applying Doppler token fallback

The fallback branch keyed on missing META_TOKEN also unconditionally rewrites META_ACCOUNT from Doppler. If the account ID is already present from env/plugin config and only the token needs Doppler (or the Doppler account secret is absent), this clears META_ACCOUNT and causes gather_meta to return null despite valid configuration. Keep the existing account value unless it is also missing.

Useful? React with 👍 / 👎.

Doppler fallback unconditionally overwrites already-resolved META_ACCOUNT

Medium Severity

When META_TOKEN is empty, the Doppler fallback block unconditionally overwrites META_ACCOUNT on line 20, even if it was already successfully resolved from an environment variable or plugin config on line 10. If the account ID exists in env/config but the token only exists in Doppler, entering this block clobbers the valid META_ACCOUNT with whatever Doppler returns — which could be empty, silently breaking Meta Ads data gathering.

 

^{Reviewed by Cursor Bugbot for commit f784157. Configure here.}