fix(codex): resolve the app-server from the port file when the variable is absent

scripts/drivers/types/codex/_app-server.sh

@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+# Where a project's codex app-server can be reached.
+#
+# codex-monitor.sh is the only writer. It records the port it bound, builds the
+# URL from it, and hands that same string to three places:
+#
+#   codex-monitor.sh:201   printf '%s' "$PORT" > "$PORT_FILE"
+#   codex-monitor.sh:212   SOCKET_URL="ws://127.0.0.1:$PORT"
+#   codex-monitor.sh:217   export AGMSG_CODEX_BRIDGE_APP_SERVER="$SOCKET_URL"
+#   codex-monitor.sh:228   exec "$REAL_CODEX" --remote "$SOCKET_URL"
+#
+# So the port file and the environment variable are two carriers of ONE value.
+# Reading the file does not invent a second way to reach the server; it
+# reconstructs the string the variable would have held, byte for byte.
+#
+# That matters because the variable does not always arrive. Under codex 0.146
+# `--remote`, an agent's shell_command runs inside the app-server process rather
+# than the TUI client, and codex-monitor.sh cannot export into that context: the
+# URL does not exist until the server's banner has been parsed, which is after
+# the server is already running. The value is not missing because nothing set
+# it — it is missing because the execution context cannot receive it.
+#
+# Callers must keep treating "no URL" as "could not ask", never as "asked and
+# got nothing". The two are different answers and only the first may fall
+# through to a weaker source.
+
+# Echo the app-server URL for <project>, or nothing.
+#
+# The environment variable wins when present: it is the value monitor exported
+# for this very process, and preferring it keeps every context that already
+# worked on exactly the path it used before.
+_agmsg_codex_app_server_url() {
+  local project="$1" port_file port
+  [ -n "$project" ] || return 0
+  if [ -n "${AGMSG_CODEX_BRIDGE_APP_SERVER:-}" ]; then
+    printf '%s' "$AGMSG_CODEX_BRIDGE_APP_SERVER"
+    return 0
+  fi
+  command -v agmsg_sha1 >/dev/null 2>&1 || return 0
+  port_file="$SKILL_DIR/run/codex-app-server.$(printf '%s' "$project" | agmsg_sha1 2>/dev/null).port"
+  port="$(cat "$port_file" 2>/dev/null || true)"
+  # Digits, and a port a TCP stack could have handed out. Digits alone are not
+  # enough on their own — a prefix of a real port (5 of 52962) is all digits and
+  # is itself a valid port, so this check cannot detect a partial read. The
+  # writer publishes atomically for that reason; this bounds the damage of
+  # anything else that could leave a stray value here.
+  #
+  # Length before magnitude: `[ -ge ]` on an unbounded digit string is a
+  # comparison on a value that may not fit.
+  case "$port" in
+    ''|*[!0-9]*|0*) return 0 ;;
+  esac
+  [ "${#port}" -le 5 ] || return 0
+  [ "$port" -ge 1 ] && [ "$port" -le 65535 ] || return 0
+  printf 'ws://127.0.0.1:%s' "$port"
+}

scripts/drivers/types/codex/_delivery.sh

@@ -99,19 +99,42 @@ agmsg_codex_probe_loaded_count() {
   [ -n "${_AGMSG_CODEX_LOADED_PROBED:-}" ] && return 0
   _AGMSG_CODEX_LOADED_PROBED=1
   _AGMSG_CODEX_LOADED_COUNT=""
+  _AGMSG_CODEX_LOADED_LIST=""
   port_file="$RUN_DIR/codex-app-server.$(printf '%s' "$project" | agmsg_sha1 2>/dev/null).port"
   port="$(cat "$port_file" 2>/dev/null || true)"
   [ -n "$port" ] || return 0
   node_bin="$(agmsg_resolve_node 2>/dev/null || true)"
   [ -n "$node_bin" ] || return 0
   { command -v "$node_bin" >/dev/null 2>&1 || [ -x "$node_bin" ]; } || return 0
-  _AGMSG_CODEX_LOADED_COUNT="$("$node_bin" "$SCRIPT_DIR/drivers/types/codex/codex-bridge.js" \
+  # Keep the ids, not only how many. The count answers "is anything loaded";
+  # telling a missing seat apart from someone else's seat needs the ids
+  # themselves, and re-asking would cost a second connect and could disagree
+  # with the first.
+  _AGMSG_CODEX_LOADED_LIST="$("$node_bin" "$SCRIPT_DIR/drivers/types/codex/codex-bridge.js" \
     --app-server "ws://127.0.0.1:$port" --print-loaded-threads \
     --connect-timeout-ms "${AGMSG_CODEX_STATUS_PROBE_TIMEOUT_MS:-1500}" \
-    --request-timeout-ms "${AGMSG_CODEX_STATUS_PROBE_TIMEOUT_MS:-1500}" 2>/dev/null | grep -c . || true)"
+    --request-timeout-ms "${AGMSG_CODEX_STATUS_PROBE_TIMEOUT_MS:-1500}" 2>/dev/null | grep . || true)"
+  _AGMSG_CODEX_LOADED_COUNT="$(printf '%s' "$_AGMSG_CODEX_LOADED_LIST" | grep -c . || true)"
   return 0
 }
 
+# How many loaded threads no role has claimed yet. Same subtraction
+# codex-record-session.sh makes when it decides whether it can identify a
+# session; the diagnostic needs it to tell "nobody seated this" apart from
+# "someone else already did". Operates on the list the probe already fetched.
+# Prints the count, or nothing when it could not be worked out. Empty is not
+# zero: zero says "someone else holds it", and a caller that reads a failure as
+# zero would state that as fact. The same distinction probe_ran keeps on the
+# seating side.
+_agmsg_codex_unseated_count() {
+  local seated
+  [ -n "${_AGMSG_CODEX_LOADED_LIST:-}" ] || return 0
+  seated="$(mktemp "${TMPDIR:-/tmp}/agmsg-dxseated.XXXXXX" 2>/dev/null)" || return 0
+  agmsg_role_session_recorded_uuids codex 2>/dev/null | grep . | sort -u > "$seated" || true
+  printf '%s\n' "$_AGMSG_CODEX_LOADED_LIST" | sort -u | comm -23 - "$seated" | grep -c . || true
+  rm -f "$seated"
+}
+
 # Why a role has no bridge. A seat (role-session record) is what every layer of
 # the monitor path requires, so its absence -- not a dead process -- is the usual
 # reason nothing is running. When the seat is missing, the loaded-thread count is
@@ -138,8 +161,19 @@ agmsg_codex_report_missing_bridge() {
       echo "  Start Codex through monitor mode in this project; the seat is recorded then."
       ;;
     1)
-      echo "Codex bridge: $team/$name has no session recorded, though one thread is loaded"
-      echo "  That combination is unexpected -- the seat is normally written for it."
+      # A loaded thread belongs to at most ONE role. If the one that is loaded is
+      # already seated elsewhere, this role having no seat is the correct state,
+      # not a surprise -- the count says how many threads are loaded, not how many
+      # are still unclaimed, and only the second would make a missing seat odd.
+      # Only an established zero earns the calmer message; an unknown falls
+      # through to the wording that asks a human to look.
+      if [ "$(_agmsg_codex_unseated_count)" = "0" ]; then
+        echo "Codex bridge: $team/$name has no session recorded (the one loaded thread is already seated by another role)"
+        echo "  Nothing to do: a thread seats one role. Start Codex for this role to give it its own."
+      else
+        echo "Codex bridge: $team/$name has no session recorded, though one thread is loaded"
+        echo "  That combination is unexpected -- the seat is normally written for it."
+      fi
       ;;
     *)
       echo "Codex bridge: $team/$name has no session recorded ($_AGMSG_CODEX_LOADED_COUNT threads loaded, none identifiable as its session)"
@@ -156,6 +190,7 @@ agmsg_delivery_runtime_status() {
   local pairs found=0 any_alive=0
   _AGMSG_CODEX_LOADED_PROBED=""
   _AGMSG_CODEX_LOADED_COUNT=""
+  _AGMSG_CODEX_LOADED_LIST=""
   pairs=$("$SCRIPT_DIR/identities.sh" "$project" "$type" 2>/dev/null || true)
 
   if [ -z "$pairs" ]; then

scripts/drivers/types/codex/codex-monitor.sh

@@ -17,6 +17,12 @@ source "$SCRIPT_DIR/../../../lib/compat.sh"
 # _agmsg_pid_alive for the app-server reuse decision below.
 # shellcheck source=../../../lib/instance-id.sh
 source "$SCRIPT_DIR/../../../lib/instance-id.sh"
+# agmsg_write_atomic: the port file is published, not just written — a reader
+# turns its contents into a URL, and a numeric PREFIX of a real port is itself
+# a valid port, so no reader-side check can tell a half-written file from a
+# good one. Only the writer can make that state unobservable.
+# shellcheck source=../../../lib/registry-lock.sh
+source "$SCRIPT_DIR/../../../lib/registry-lock.sh"
 
 PROJECT="$(pwd)"
 SOCKET_PATH=""
@@ -198,7 +204,7 @@ if [ -z "$PORT" ]; then
     rm -f "$SERVER_PID" "$VERSION_FILE"
     exec_plain_codex
   fi
-  printf '%s' "$PORT" > "$PORT_FILE"
+  agmsg_write_atomic "$PORT_FILE" "$PORT"
   # Stamp the version that owns this server so a later launch from a different
   # codex build recreates it instead of reusing a stale one.
   printf '%s' "$CODEX_VERSION" > "$VERSION_FILE"

scripts/drivers/types/codex/codex-record-session.sh

@@ -49,6 +49,10 @@ export SKILL_DIR
 . "$SKILL_DIR/scripts/lib/storage.sh"
 # shellcheck disable=SC1091
 . "$SKILL_DIR/scripts/lib/role-session.sh"
+# shellcheck disable=SC1091
+. "$SKILL_DIR/scripts/lib/hash.sh"
+# shellcheck disable=SC1091
+. "$SCRIPT_DIR/_app-server.sh"
 
 # Poison-record guard (best-effort bias: record nothing when unsure). A mangled
 # <project> argument -- e.g. the lone `\` a PowerShell-parsed \"$PWD\" collapses
@@ -104,7 +108,12 @@ fi
 # be identified, and letting a weaker source overrule it is how a wrong thread
 # gets seated.
 probe_ran=0
-if [ -z "$thread" ] && [ -n "${AGMSG_CODEX_BRIDGE_APP_SERVER:-}" ]; then
+# The URL, not the environment variable. Under codex 0.146 `--remote` this script
+# runs inside the app-server process, which is a context codex-monitor.sh cannot
+# export into, so gating on the variable meant this probe never ran on the very
+# path #579 was about. The port file carries the same string (see _app-server.sh).
+app_server="$(_agmsg_codex_app_server_url "$PROJECT")"
+if [ -z "$thread" ] && [ -n "$app_server" ]; then
   # shellcheck disable=SC1091
   . "$SKILL_DIR/scripts/lib/node.sh"
   node_bin="$(agmsg_resolve_node 2>/dev/null || true)"
@@ -114,7 +123,7 @@ if [ -z "$thread" ] && [ -n "${AGMSG_CODEX_BRIDGE_APP_SERVER:-}" ]; then
     if [ -n "$loaded_file" ] && [ -n "$seated_file" ]; then
       # Exit status, not output: an empty list is a valid answer ("nothing is
       # loaded"), while a failure to reach the app-server is not an answer at all.
-      if "$node_bin" "$SCRIPT_DIR/codex-bridge.js" --app-server "$AGMSG_CODEX_BRIDGE_APP_SERVER" \
+      if "$node_bin" "$SCRIPT_DIR/codex-bridge.js" --app-server "$app_server" \
            --print-loaded-threads >"$loaded_file.raw" 2>/dev/null; then
         probe_ran=1
       fi

tests/test_codex_monitor.bats

@@ -381,3 +381,15 @@ EOF
   grep -q 'plain-codex <--remote> <ws://127\.0\.0\.1:[0-9][0-9]*>' "$CALL_LOG"
   [[ "$output" != *"did not report a listening port"* ]]
 }
+
+@test "codex monitor: the port file is published atomically, never written in place" {
+  # A reader turns this file's contents into a URL, and a numeric PREFIX of a
+  # real port is itself a valid port — 5296 while 52962 is being written names a
+  # DIFFERENT app-server, possibly another project's, which would answer and let
+  # its thread be seated here. No reader-side check can tell those apart, so the
+  # partial state has to be unobservable rather than filtered.
+  local src="$SCRIPTS/drivers/types/codex/codex-monitor.sh"
+  grep -q 'agmsg_write_atomic "$PORT_FILE"' "$src"
+  # No truncating redirect to the published path.
+  ! grep -qE '>[[:space:]]*"\$PORT_FILE"' "$src"
+}

tests/test_codex_resume.bats

@@ -347,3 +347,83 @@ fake_node_failing() {
     bash "$TYPES/codex/codex-record-session.sh" team alice "$proj" )
   [ "$(recorded_uuid team alice)" = "thread-A" ]
 }
+
+# --- seating without the app-server environment variable (#583 follow-up) ---
+#
+# The variable does not arrive on the path #579 is about. Under codex 0.146
+# `--remote` this script runs inside the app-server process, and codex-monitor.sh
+# cannot export into that context: the URL does not exist until the server's
+# banner has been parsed, which is after the server is running. The port file
+# carries the same string, so seating has to work from it alone.
+
+record_with_loaded_via_port_file() {   # <ids-file> <team> <agent> <project>
+  local hash
+  # shellcheck disable=SC1091
+  source "$SKILL_DIR/scripts/lib/hash.sh"
+  hash="$(printf '%s' "$4" | agmsg_sha1)"
+  mkdir -p "$TEST_SKILL_DIR/run"
+  printf '1' > "$TEST_SKILL_DIR/run/codex-app-server.$hash.port"
+  ( unset CODEX_THREAD_ID AGMSG_CODEX_BRIDGE_APP_SERVER
+    AGMSG_NODE="$(fake_node_printing "$1")" \
+      bash "$TYPES/codex/codex-record-session.sh" "$2" "$3" "$4" )
+}
+
+@test "codex record: seats from the port file when the app-server variable never arrives" {
+  local proj ids; proj="$(mktemp -d)"; ids="$TEST_SKILL_DIR/loaded.txt"
+  printf 'thr-seated\nthr-unclaimed\n' > "$ids"
+  source "$SKILL_DIR/scripts/lib/role-session.sh"
+  agmsg_role_session_record team bob thr-seated "$proj" codex
+  record_with_loaded_via_port_file "$ids" team alice "$proj"
+  [ "$(recorded_uuid team alice)" = "thr-unclaimed" ]
+}
+
+@test "codex record: no port file and no variable records nothing, it does not guess" {
+  # Fail closed. Without a way to ask, the answer is "could not ask" -- never
+  # "asked and found nothing" -- so no weaker signal may seat a thread here.
+  local proj ids; proj="$(mktemp -d)"; ids="$TEST_SKILL_DIR/loaded.txt"
+  printf 'thr-unclaimed\n' > "$ids"
+  ( unset CODEX_THREAD_ID AGMSG_CODEX_BRIDGE_APP_SERVER
+    AGMSG_NODE="$(fake_node_printing "$ids")" \
+      bash "$TYPES/codex/codex-record-session.sh" team alice "$proj" )
+  [ -z "$(recorded_uuid team alice)" ]
+}
+
+@test "codex record: a half-written port file is not turned into a URL" {
+  local proj ids hash; proj="$(mktemp -d)"; ids="$TEST_SKILL_DIR/loaded.txt"
+  printf 'thr-unclaimed\n' > "$ids"
+  # shellcheck disable=SC1091
+  source "$SKILL_DIR/scripts/lib/hash.sh"
+  hash="$(printf '%s' "$proj" | agmsg_sha1)"
+  mkdir -p "$TEST_SKILL_DIR/run"
+  printf 'not-a-port' > "$TEST_SKILL_DIR/run/codex-app-server.$hash.port"
+  ( unset CODEX_THREAD_ID AGMSG_CODEX_BRIDGE_APP_SERVER
+    AGMSG_NODE="$(fake_node_printing "$ids")" \
+      bash "$TYPES/codex/codex-record-session.sh" team alice "$proj" )
+  [ -z "$(recorded_uuid team alice)" ]
+}
+
+@test "codex record: a digits-only value outside the port range is not turned into a URL" {
+  # Digits alone do not make a port. A prefix of a real port is also all digits,
+  # which is why the writer publishes atomically — this bounds what anything
+  # else could leave behind.
+  local proj ids hash bad
+  proj="$(mktemp -d)"; ids="$TEST_SKILL_DIR/loaded.txt"
+  printf 'thr-unclaimed\n' > "$ids"
+  # shellcheck disable=SC1091
+  source "$SKILL_DIR/scripts/lib/hash.sh"
+  hash="$(printf '%s' "$proj" | agmsg_sha1)"
+  mkdir -p "$TEST_SKILL_DIR/run"
+  for bad in 0 65536 999999 00042; do
+    printf '%s' "$bad" > "$TEST_SKILL_DIR/run/codex-app-server.$hash.port"
+    ( unset CODEX_THREAD_ID AGMSG_CODEX_BRIDGE_APP_SERVER
+      AGMSG_NODE="$(fake_node_printing "$ids")" \
+        bash "$TYPES/codex/codex-record-session.sh" team alice "$proj" )
+    [ -z "$(recorded_uuid team alice)" ] || { echo "seated from port '$bad'"; false; }
+  done
+  # …and the boundary values that ARE ports still work.
+  printf '65535' > "$TEST_SKILL_DIR/run/codex-app-server.$hash.port"
+  ( unset CODEX_THREAD_ID AGMSG_CODEX_BRIDGE_APP_SERVER
+    AGMSG_NODE="$(fake_node_printing "$ids")" \
+      bash "$TYPES/codex/codex-record-session.sh" team alice "$proj" )
+  [ "$(recorded_uuid team alice)" = "thr-unclaimed" ]
+}

tests/test_delivery.bats

@@ -2607,3 +2607,33 @@ JSON
   [ "$(wc -c < "$calls" | tr -d ' ')" = "1" ]
   [[ "$output" == *"2 threads loaded"* ]]
 }
+
+@test "delivery status (codex): a thread already seated elsewhere is not called unexpected" {
+  # A loaded thread seats ONE role. When the only loaded thread belongs to alice,
+  # bob having no seat is the correct state — the old wording called it
+  # unexpected because it counted loaded threads rather than unclaimed ones.
+  bash "$SCRIPTS/join.sh" team alice codex "$TEST_PROJECT" >/dev/null
+  bash "$SCRIPTS/join.sh" team bob codex "$TEST_PROJECT" >/dev/null
+  bash "$SCRIPTS/delivery.sh" set monitor codex "$TEST_PROJECT" >/dev/null
+  mkdir -p "$TEST_SKILL_DIR/run"
+
+  # shellcheck disable=SC1091
+  source "$SCRIPTS/lib/hash.sh"
+  # The record has to land in the tree delivery.sh will read, so the helper needs
+  # the test's skill dir rather than the one it would derive for itself.
+  export SKILL_DIR="$TEST_SKILL_DIR"
+  # shellcheck disable=SC1091
+  source "$SCRIPTS/lib/role-session.sh"
+  agmsg_role_session_record team alice thr-alice "$TEST_PROJECT" codex
+  [ -n "$(agmsg_role_session_uuid team alice)" ]
+  printf '1' > "$TEST_SKILL_DIR/run/codex-app-server.$(printf '%s' "$TEST_PROJECT" | agmsg_sha1).port"
+
+  local fake="$TEST_SKILL_DIR/fake-node-loaded"
+  { printf '#!/usr/bin/env bash\n'; printf 'printf %%s\\\\n thr-alice\n'; } > "$fake"
+  chmod +x "$fake"
+
+  AGMSG_NODE="$fake" run bash "$SCRIPTS/delivery.sh" status codex "$TEST_PROJECT"
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"Codex bridge: team/bob has no session recorded (the one loaded thread is already seated by another role)"* ]]
+  [[ "$output" != *"That combination is unexpected"* ]]
+}