fix(issue-5): address phase 1 cross-review findings (R1, R2, F-1)

Three reviewers examined fc0bd96..8def235. Codex and Claude each reproduced a
defect; all three findings were re-verified here before being accepted, and none
were rebutted.

R1 (codex) — the shell helpers ran `python -m lib_py.delegation.cli`. For -m,
sys.path[0] is the working directory, which therefore beats PYTHONPATH: with cwd
inside a tree containing lib_py, the live package won even with the frozen path
listed first. The documented B-13 guarantee for this path did not hold.
Reproduced, then replaced with an explicit bootstrap that inserts SKILL_DIR
itself. cwd is left alone, because the registry and audit-log paths are relative
to it. The file-based shim path was never affected.

R2 (codex) — the persistent connection was reused on host and port alone. Two
jobs sharing an endpoint but carrying different credentials or TLS settings
silently published over the first connection. Reproduced (the client factory ran
once, under the first account, with TLS off). The guard now compares an
immutable snapshot of the whole connection identity and names only the differing
field, never its value.

F-1 (claude) — paho queues a publish issued while disconnected and raises
RuntimeError from wait_for_publish. RuntimeError is deliberately outside the
retry tuple, so the one state a persistent connection exists to survive, the gap
between a drop and paho's reconnect, failed on the first attempt. Confirmed
against paho 2.1.0. That single condition is now translated into a retryable
error; the tuple is unchanged, so genuine programming errors are still not
retried.

Two fixtures were hiding these: FakeInfo had no rc and never raised. It now
mirrors paho's contract, which is what makes the new tests meaningful.

Also folds in the duplication both Claude (O-1) and I had flagged: the CLI path
kept its own copy of connect-publish-disconnect plus its own timeout constants.
_publish_once now delegates to OneShotPublisher.publish_raw, so a transport fix
lands once. Having two copies is exactly why these bugs existed in one of them.

Documentation corrected rather than defended: the benchmark's rationale for
keeping three forks was imprecise (the || true already guarantees rc 0; the
lookup buys quietness and legacy parity), SKILL.md no longer claims PYTHONPATH
decides the frozen path, and the daemon's directory creation is recorded as a
deliberate difference. Trailing whitespace inherited by the relocated file is
stripped, so the commit range is now clean too.

Verified: each regression fails against the pre-fix code and passes after
(R1 1, R2 4, F-1 1). 66 phase-1 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UrDNJkHmm18uu7n7JwV5Qd

.agents/skills/lib.sh

@@ -1949,6 +1949,23 @@ _start_watchdog_legacy() {
 # loop run frozen delegation code.
 # ---------------------------------------------------------------------------
 
+# _delegation_py <cli-args...>
+# Run the delegation CLI from *this* tree.
+#
+# NOT `python -m`: for -m, sys.path[0] is the current directory, which therefore
+# beats PYTHONPATH. A frozen loop whose cwd happened to contain a lib_py package
+# would silently execute live code, losing B-13. An explicit insert of SKILL_DIR
+# cannot be shadowed that way. cwd is deliberately left alone, because the
+# registry and audit-log paths are relative to it.
+_delegation_py() {
+  PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" "$(_delegate_py_bin)" -c '
+import sys
+sys.path.insert(0, sys.argv[1])
+from lib_py.delegation.cli import main
+sys.exit(main(sys.argv[2:]))
+' "$SKILL_DIR" "$@"
+}
+
 # delegate_submit_job <prompt> <agent> <agent_session>
 # Register a job. Prints the new JID on stdout and nothing else.
 delegate_submit_job() {
@@ -1960,8 +1977,7 @@ delegate_submit_job() {
     echo "ERROR: multi-agent-mux-delegate-job registry.py not found under .agents/skills/" >&2
     return 1
   fi
-  PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" "$(_delegate_py_bin)" -m lib_py.delegation.cli \
-    submit --prompt "$prompt" --agent "$agent" --agent-session "$session"
+  _delegation_py submit --prompt "$prompt" --agent "$agent" --agent-session "$session"
 }
 
 # delegate_publish_event <job_id> <event> [detail]
@@ -1974,8 +1990,7 @@ delegate_publish_event() {
   local pub
   pub="$(_delegate_script publish_event.py)"
   [ -n "$pub" ] && [ -f "$pub" ] || return 0
-  PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" "$(_delegate_py_bin)" -m lib_py.delegation.cli \
-    publish --job "$job_id" --event "$event" --detail "$detail" || true
+  _delegation_py publish --job "$job_id" --event "$event" --detail "$detail" || true
 }
 
 # ensure_subscriber_daemon <job_id> [workdir]
@@ -1983,8 +1998,7 @@ delegate_publish_event() {
 # Prints the PID(s) on stdout, exactly as start_watchdog always did.
 ensure_subscriber_daemon() {
   if [ "${MAM_DELEGATION_PY:-1}" = "0" ]; then _start_watchdog_legacy "$@"; return $?; fi
-  PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" "$(_delegate_py_bin)" -m lib_py.delegation.cli \
-    ensure-daemon --job "$1" --workdir "${2:-$PWD}"
+  _delegation_py ensure-daemon --job "$1" --workdir "${2:-$PWD}"
 }
 
 # start_watchdog — retained name; create_session.sh and operator docs use it.

.agents/skills/lib_py/delegation/bench.py

@@ -27,10 +27,22 @@ from lib_py.delegation.publisher import OneShotPublisher, PersistentPublisher
 
 
 class FakeInfo(object):
-    def __init__(self, published=True):
+    """Stand-in for paho's MQTTMessageInfo, including its failure contract.
+
+    paho carries an ``rc`` and raises RuntimeError from ``wait_for_publish``
+    when the publish could not be handed to a live connection. The earlier
+    version of this fake had neither, which is precisely why a real defect in
+    that path stayed invisible while every test passed.
+    """
+
+    def __init__(self, published=True, rc=0):
         self._published = published
+        self.rc = rc
 
     def wait_for_publish(self, timeout=None):
+        if self.rc:
+            raise RuntimeError(
+                "Message publish failed: The client is not currently connected.")
         return None
 
     def is_published(self):

.agents/skills/lib_py/delegation/daemon.py

@@ -50,6 +50,12 @@ def _ignore_sighup():  # pragma: no cover - runs in the forked child
 
 
 def spawn(monitor_script: str, workdir: str) -> int:
+    """Start the subscriber detached and return its pid.
+
+    One deliberate difference from the shell original: the log directory is
+    created if missing. The legacy `>>` redirect failed outright when `.mam/`
+    did not exist, so this is strictly more robust, not a contract change.
+    """
     log_file = os.path.join(workdir, LOG_RELPATH)
     os.makedirs(os.path.dirname(log_file), exist_ok=True)
     log_handle = open(log_file, "ab")

.agents/skills/lib_py/delegation/mqtt_common.py

@@ -38,12 +38,12 @@ _warned_coexistence_env = False
 
 def _load_dotenv(workspace_dir: Optional[str] = None) -> None:
     """Load .mam.env (or .env fallback) from workspace if it exists.
-    
+
     Only sets vars that are not already in os.environ
     (i.e. OS env takes precedence over env files).
     """
     global _warned_deprecated_env, _warned_coexistence_env
-    
+
     # 1. Check explicit MAM_ENV_FILE override
     explicit_file = os.environ.get("MAM_ENV_FILE")
     if explicit_file:

.agents/skills/lib_py/delegation/publish_event.py

@@ -27,7 +27,6 @@ import hmac
 import json
 import logging
 import sys
-import time
 from typing import Any, Dict, Optional
 
 from lib_py.delegation import mqtt_common
@@ -38,7 +37,6 @@ from lib_py.delegation.mqtt_common import (
     broker_config_from_job,
     events_topic_for,
     load_job,
-    make_client,
     next_seq,
     with_retry,
 )
@@ -55,8 +53,13 @@ EVENT_TO_STATUS = {
     "cancelled": "cancelled",
 }
 
-CONNECT_ACK_TIMEOUT = 10  # seconds to wait for CONNACK
-PUBLISH_ACK_TIMEOUT = 5   # seconds to wait for QoS-1 PUBACK
+# Re-exported from the transport module, which owns the single implementation.
+# Two copies of these numbers meant tuning one silently diverged from the other.
+from lib_py.delegation.publisher import (  # noqa: F401  (re-exported for callers)
+    CONNECT_ACK_TIMEOUT,
+    PUBLISH_ACK_TIMEOUT,
+    OneShotPublisher,
+)
 
 
 def build_payload(
@@ -91,37 +94,12 @@ def build_payload(
 def _publish_once(config, topic: str, body: bytes, retain: bool) -> None:
     """Connect, publish one QoS-1 message, wait for the broker ACK, disconnect.
 
-    Raises on any failure so ``with_retry`` can re-run the whole sequence (a
-    fresh connection per attempt is the robust choice for a PoC)."""
-    client = make_client("publisher", config)
-    connected = {"rc": None}
-
-    def on_connect(_c, _u, _flags, reason_code, _props):
-        connected["rc"] = reason_code
-
-    client.on_connect = on_connect
-    client.connect(config.host, config.port, config.keepalive)
-    client.loop_start()
-    try:
-        # Wait for CONNACK so we fail fast on auth/TLS errors.
-        deadline = time.monotonic() + CONNECT_ACK_TIMEOUT
-        while connected["rc"] is None and time.monotonic() < deadline:
-            time.sleep(0.05)
-        if connected["rc"] is None:
-            raise TimeoutError("no CONNACK from broker")
-        if mqtt_common.reason_code_value(connected["rc"]) != 0:
-            raise ConnectionError(f"broker refused connection: rc={connected['rc']}")
-
-        info = client.publish(topic, payload=body, qos=1, retain=retain)
-        info.wait_for_publish(timeout=PUBLISH_ACK_TIMEOUT)
-        if not info.is_published():
-            raise TimeoutError("publish not acknowledged within timeout")
-    finally:
-        client.loop_stop()
-        try:
-            client.disconnect()
-        except Exception:  # pragma: no cover - disconnect best effort
-            pass
+    Delegates to the transport module so the CLI path and the service path share
+    one implementation. Keeping a second copy here is what let the persistent
+    transport's handshake and not-connected bugs exist in only one of them.
+    Raises on any failure so ``with_retry`` can re-run the whole sequence.
+    """
+    OneShotPublisher().publish_raw(config, topic, body, retain)
 
 
 class PreparedEvent(object):

.agents/skills/lib_py/delegation/publisher.py

@@ -12,7 +12,9 @@ from __future__ import annotations
 
 import logging
 import time
-from typing import Optional, Tuple
+from typing import Any, Optional, Tuple
+
+import paho.mqtt.client as mqtt
 
 from lib_py.delegation import mqtt_common
 
@@ -22,6 +24,25 @@ CONNECT_ACK_TIMEOUT = 10  # seconds to wait for CONNACK
 PUBLISH_ACK_TIMEOUT = 5   # seconds to wait for QoS-1 PUBACK
 
 
+def _send(client, topic: str, body: bytes, retain: bool) -> None:
+    """Publish one QoS-1 message and wait for the broker ACK.
+
+    paho queues a publish issued while disconnected and returns
+    ``MQTT_ERR_NO_CONN``; ``wait_for_publish`` then raises ``RuntimeError``,
+    which is deliberately NOT in the retry tuple (a RuntimeError normally means a
+    programming error and must not be retried blindly). Left alone, that turned
+    the one state a persistent connection exists to survive -- the gap between a
+    drop and paho's reconnect -- into a single-attempt failure. Detect that one
+    condition explicitly and raise a retryable error instead.
+    """
+    info = client.publish(topic, payload=body, qos=1, retain=retain)
+    if getattr(info, "rc", mqtt.MQTT_ERR_SUCCESS) == mqtt.MQTT_ERR_NO_CONN:
+        raise ConnectionError("client is not connected (paho is reconnecting)")
+    info.wait_for_publish(timeout=PUBLISH_ACK_TIMEOUT)
+    if not info.is_published():
+        raise TimeoutError("publish not acknowledged within timeout")
+
+
 class _BasePublisher(object):
     """Shared retry wrapper so both transports report outcomes identically."""
 
@@ -56,7 +77,15 @@ class OneShotPublisher(_BasePublisher):
         self.connect_count = 0
 
     def publish(self, prepared):
-        client = self.client_factory("publisher", prepared.config)
+        self.publish_raw(prepared.config, prepared.topic, prepared.body, prepared.retain)
+
+    def publish_raw(self, config, topic: str, body: bytes, retain: bool) -> None:
+        """Connect, publish, wait for the ACK, disconnect.
+
+        The single implementation of one-shot publishing: ``publish_event``'s
+        CLI path calls straight into this, so the two entry points cannot drift.
+        """
+        client = self.client_factory("publisher", config)
         self.connect_count += 1
         connected = {"rc": None}
 
@@ -64,7 +93,7 @@ class OneShotPublisher(_BasePublisher):
             connected["rc"] = reason_code
 
         client.on_connect = on_connect
-        client.connect(prepared.config.host, prepared.config.port, prepared.config.keepalive)
+        client.connect(config.host, config.port, config.keepalive)
         client.loop_start()
         try:
             deadline = time.monotonic() + CONNECT_ACK_TIMEOUT
@@ -74,11 +103,7 @@ class OneShotPublisher(_BasePublisher):
                 raise TimeoutError("no CONNACK from broker")
             if mqtt_common.reason_code_value(connected["rc"]) != 0:
                 raise ConnectionError("broker refused connection: rc={}".format(connected["rc"]))
-            info = client.publish(prepared.topic, payload=prepared.body,
-                                  qos=1, retain=prepared.retain)
-            info.wait_for_publish(timeout=PUBLISH_ACK_TIMEOUT)
-            if not info.is_published():
-                raise TimeoutError("publish not acknowledged within timeout")
+            _send(client, topic, body, retain)
         finally:
             client.loop_stop()
             try:
@@ -100,6 +125,7 @@ class PersistentPublisher(_BasePublisher):
         self.connect_count = 0
         self._client = None
         self._config = None
+        self._fingerprint_bound = None
 
     def __enter__(self):
         return self.open()
@@ -111,12 +137,35 @@ class PersistentPublisher(_BasePublisher):
     def open(self):
         return self
 
+    @staticmethod
+    def _fingerprint(config) -> tuple:
+        """An immutable snapshot of everything that decides *which connection*.
+
+        Endpoint alone is not enough: two jobs can share a host and port while
+        carrying different credentials or TLS material, and reusing the first
+        connection would publish under the wrong principal or silently drop a
+        requested TLS setting. Snapshotting also means later mutation of the
+        original config object cannot change what the guard believes is bound.
+        """
+        fields = ("host", "port", "username", "password", "tls", "ca_certs",
+                  "certfile", "keyfile", "keepalive", "client_id_prefix")
+        return tuple((name, getattr(config, name, None)) for name in fields)
+
+    @staticmethod
+    def _describe_mismatch(bound: tuple, incoming: tuple) -> str:
+        """Name the differing fields without ever echoing their values."""
+        differing = [name for (name, old), (_n, new) in zip(bound, incoming) if old != new]
+        return ", ".join(differing) or "connection settings"
+
     def _ensure_client(self, config):
         if self._client is not None:
-            if (config.host, config.port) != (self._config.host, self._config.port):
+            incoming = self._fingerprint(config)
+            if incoming != self._fingerprint_bound:
                 raise ValueError(
-                    "PersistentPublisher is bound to {}:{}; refusing an event for {}:{}".format(
-                        self._config.host, self._config.port, config.host, config.port)
+                    "PersistentPublisher is bound to {}:{}; refusing an event whose "
+                    "{} differ".format(
+                        self._config.host, self._config.port,
+                        self._describe_mismatch(self._fingerprint_bound, incoming))
                 )
             return self._client
 
@@ -151,6 +200,7 @@ class PersistentPublisher(_BasePublisher):
             raise
         self._client = client
         self._config = config
+        self._fingerprint_bound = self._fingerprint(config)
         return client
 
     @staticmethod
@@ -163,12 +213,10 @@ class PersistentPublisher(_BasePublisher):
 
     def publish(self, prepared):
         client = self._ensure_client(prepared.config)
-        info = client.publish(prepared.topic, payload=prepared.body,
-                              qos=1, retain=prepared.retain)
-        info.wait_for_publish(timeout=PUBLISH_ACK_TIMEOUT)
-        if not info.is_published():
-            # Surfaced to the caller's with_retry; paho reconnects underneath.
-            raise TimeoutError("publish not acknowledged within timeout")
+        # Both failure shapes (queued-while-disconnected and no PUBACK) are raised
+        # as retryable errors by _send, so the caller's with_retry can ride out a
+        # reconnect instead of recording a one-attempt failure.
+        _send(client, prepared.topic, prepared.body, prepared.retain)
 
     def close(self):
         if self._client is None:
@@ -178,3 +226,4 @@ class PersistentPublisher(_BasePublisher):
         finally:
             self._client = None
             self._config = None
+            self._fingerprint_bound = None

.agents/skills/multi-agent-mux-delegate-job/SKILL.md

@@ -84,14 +84,25 @@ Which tree wins, by execution context:
 
 | Context | Resolved by | Tree that wins |
 |---|---|---|
-| Loop process and its children (frozen `lib.sh` sourced) | `PYTHONPATH` | the frozen snapshot |
+| Loop process and its children (frozen `lib.sh` sourced) | the helper's explicit bootstrap from `SKILL_DIR` | the frozen snapshot |
 | Agent session running a **frozen** script path while `PYTHONPATH` still points live | the shim, from its own location | the frozen snapshot (B-13 preserved) |
 | Live helpers (create/stop/resume/reconcile) | `PYTHONPATH` or the shim (same tree) | the live tree |
 | Standalone, no `PYTHONPATH` | the shim's fallback append | the shim's own tree |
 
-`lib.sh` calls the package through `python -m lib_py.delegation.cli`. Setting
-`MAM_DELEGATION_PY=0` switches the shell helpers back to their preserved legacy
-bodies with no redeploy.
+`lib.sh` calls the package through `_delegation_py`, which inserts its own
+`SKILL_DIR` at the front of the import path. It deliberately does **not** use
+`python -m`: for `-m` the current directory precedes `PYTHONPATH`, so a frozen
+loop whose working directory happened to contain a `lib_py` package would have
+silently executed live code. The working directory is left untouched, because
+the registry and audit-log paths are relative to it.
+
+Setting `MAM_DELEGATION_PY=0` switches the shell helpers back to their preserved
+legacy bodies with no redeploy.
+
+One in-process caveat: the package binds once per process and is never rebound,
+so do not import a frozen shim after a live `lib_py.delegation` is already bound
+inside the same interpreter. Production never does this — each delegated job is
+a fresh interpreter — but a test harness could.
 
 ---