# Cross-Code Review Report: Job 8e92d62d

## Milestone M1 / Track 0 — Fault Tolerance Implementation

**Reviewer**: cline (session: herdr:canary-projects-multi-agent-mux-creator-cline)
**Date**: 2026-08-20
**Scope**: Cross-code review of M1/Track 0 implementation (B-14, B-15, F-4) + 10 regression guards (G-1~G-10)

---

## 1. Review Summary

### 1.1 Files Changed (5 files, +420 / -38 lines)

| File | Lines | Purpose |
|------|-------|---------|
| `publish_event.py` | +17/-7 | B-14: Local disk updates before exit on publish failure |
| `job_subscriber.py` | +135/-38 | B-15: Disk fallback polling + rc=3 broker-down exit |
| `multi-agent-mux-delegate-job` | +9/-1 | F-4: rc=3 → broker_unavailable mapping + disk recheck |
| `tests/test_tier1_unit.py` | +289/0 | G-1~G-10 regression guards |
| `implementation_plan.md` | +8/-8 | M1 checkboxes `[ ]` → `[x]` |

### 1.2 Verification Performed

| Check | Result |
|-------|--------|
| `py_compile` on all 3 Python files | ✅ COMPILE OK |
| `bash -n` on delegate-job script | ✅ BASH SYNTAX OK |
| G-1~G-10 guard tests (10 tests) | ✅ 10/10 PASSED (1.42s) |
| `test_tier1_unit.py` full (45 tests) | ✅ 45/45 PASSED (8.17s) |
| `test_deploy_freshness.py` (13 tests) | ✅ 13/13 PASSED |
| `test_o2_race_free_lock.py` (22 tests) | ✅ 22/22 PASSED |
| Fast subset (test_sanity, workspace_scope, o3, a4, o1) | ✅ 58/58 PASSED (12.14s) |
| `pytest --collect-only` total | ✅ 290 tests collected (matches plan claim 280→290) |
| Full suite end-to-end | ⚠️ Exceeds 30s timeout (tier2/3/4 require broker/subprocess) |
| Codebase accuracy claims (line refs, function signatures) | ✅ Verified (see §4) |

---

## 2. Detailed Review by Step

### 2.1 Step 1 — `publish_event.py` B-14: Status Sync Before Exit (G-1~G-4)

**Requirement**: Local disk updates (registry status + audit log) must ALWAYS be performed before exiting on network publish failure (rc=2).

**Implementation** (`publish_event.py:195-232`):
```python
publish_ok = True
publish_error: Optional[str] = None
try:
    publish(config, topic, body, retain)
except Exception as exc:
    publish_ok = False
    publish_error = str(exc)
    logger.error(...)

# Audit log — ALWAYS runs (before return 2)
mqtt_common.append_event(job_id, {
    "event": "published",
    ...
    "published": publish_ok,
    "publish_error": publish_error,
})

# Registry status sync — ALWAYS runs (before return 2)
registry.append_event(job_id, args.registry_dir, payload)
new_status = EVENT_TO_STATUS.get(args.event)
if new_status:
    mqtt_common.update_job_status(...)  # also mirrors to status.json

if not publish_ok:
    return 2   # ← exit AFTER disk persistence
return 0
```

**Verdict**: ✅ **Correct**. The original code had `return 2` inside the `except` block, which skipped the audit log and status sync. The new code moves `return 2` to after all disk persistence operations. The seq consumption policy is maintained (seq is consumed even on failure) and documented with a clear comment. The `published` and `publish_error` fields in the audit record provide full traceability.

**Test Coverage**:
- G-1: Verifies `registry.load_job().status == "completed"` after publish failure → ✅
- G-2: Verifies audit log has `published=False` and `publish_error is not None` → ✅
- G-3: Verifies `published=True` and `publish_error is None` on success → ✅
- G-4: Verifies seq advances (1→2) across failed-then-successful publish → ✅

### 2.2 Step 2 — `job_subscriber.py` B-15: Disk Fallback (G-5~G-8)

**Requirement**: Poll local disk status every 3s on `queue.Empty`; cleanly exit (rc=0 on completed, rc=1 on error) via disk-fallback when terminal state is reached.

**Implementation**:
- `_check_disk_fallback()` function added (lines 60-92): reads `load_job().status` from registry, falls back to `read_logged_status()` from audit logs.
- Called at 5 points: (1) before connecting, (2) on broker connect failure, (3) on wall-clock timeout, (4) on idle timeout, (5) every 3.0s on `queue.Empty`.
- `main()` refactored: `_run_subscriber()` contains the core logic; `main()` wraps it with a catch-all `try/except` returning rc=3 on unexpected errors.
- `connected` flag guards `finally` cleanup (only stops/disconnects if actually connected).

**Verdict**: ⚠️ **Functionally correct for primary path; secondary fallback path has a bug (M-1)**. The registry JSON fallback works and all tests pass. However, the status.json fallback via `read_logged_status()` is dead code due to a type mismatch (see Finding M-1).

**Test Coverage**:
- G-5: Broker down + disk `status=completed` → rc=0 → ✅
- G-6: Broker down + disk `status=completed` → stdout contains `disk-fallback` tag → ✅
- G-7: Broker down + disk `status=error` → rc=1 → ✅
- G-8: `--wait-any` with 1 completed + 1 running → does NOT exit early (rc=2 timeout) → ✅

### 2.3 Step 3 — `multi-agent-mux-delegate-job` F-4: rc=3 Separation (G-9~G-10)

**Requirement**: Handle job_subscriber rc=3 (broker connection failure) and map to `broker_unavailable`, with disk status recheck.

**Implementation** (lines 340-346):
```bash
elif [[ $sub_rc -eq 3 ]]; then
  job_status="broker_unavailable"
  local disk_st
  disk_st="$PY" -c "import json, os; p=os.path.join('$REGISTRY_DIR', '$JOB_ID.json'); \
    print(json.load(open(p)).get('status','')) if os.path.exists(p) else print('')" 2>/dev/null || true"
  if [[ "$disk_st" == "completed" || "$disk_st" == "error" ]]; then
    job_status="$disk_st"
  fi
```

Also at line 179: readiness check now accepts `sub_exit -eq 3` as "ready" (subscriber resolved via disk fallback before broker connected).

**Verdict**: ✅ **Correct**. The rc=3 branch properly separates infrastructure failures from job errors. The inline Python disk-status check correctly reads the registry JSON. The fallback to disk status prevents false `broker_unavailable` when the subscriber already resolved the terminal state via disk fallback.

**Test Coverage**:
- G-9: Broker down + no terminal on disk → rc=3 → ✅
- G-10: Static assertion that delegate script contains `elif [[ $sub_rc -eq 3 ]]` and `job_status=broker_unavailable` → ✅

---

## 3. Findings

### M-1 (Medium): `read_logged_status()` return type mismatch — status.json fallback is dead code

**Location**: `job_subscriber.py:73-77` in `_check_disk_fallback()`

**Description**:
```python
# Line 75: read_logged_status returns Optional[Dict[str, Any]], NOT a string
disk_status = mqtt_common.read_logged_status(jid, mqtt_common.get_logs_dir())
```
`mqtt_common.read_logged_status()` (mqtt_common.py:559) returns `Optional[Dict[str, Any]]` — a dict like `{"job_id": "...", "status": "completed", "updated_at": "..."}` or `None`.

The code then checks:
```python
if disk_status in ("completed", "error", "cancelled"):  # Line 79
```
This compares a **dict** (or `None`) against a tuple of **strings** → **always `False`**.

The correct usage pattern (seen in `mqtt_common.py:601-602`) is:
```python
status_rec = read_logged_status(d.name, logs_dir)
if status_rec:
    ... status_rec.get("status") ...
```

**Impact**: The secondary fallback path (status.json when registry JSON is unavailable/corrupted) never resolves a terminal status. The primary path (`load_job().get("status")`) works correctly, so disk fallback still functions via the registry JSON. All tests pass because they set `job["status"]` directly in the registry JSON and never exercise the status.json fallback.

**Fix** (one-line change):
```python
# Before:
disk_status = mqtt_common.read_logged_status(jid, mqtt_common.get_logs_dir())
# After:
status_rec = mqtt_common.read_logged_status(jid, mqtt_common.get_logs_dir())
disk_status = status_rec.get("status") if status_rec else None
```

**Severity**: Medium — reduces resilience of the B-15 fallback but does not break primary functionality.

### M-2 (Low): `main()` catch-all exception handler masks unexpected errors as rc=3

**Location**: `job_subscriber.py:331-335`

```python
try:
    return _run_subscriber(args)
except Exception as exc:
    logger.error("subscriber fatal error: %s", exc)
    return 3
```

Any unexpected exception (e.g., `KeyError`, `AttributeError`, bug in event loop) gets mapped to rc=3 (`broker_unavailable`), which the delegate script then interprets as an infrastructure failure. This could mask real bugs during development. The error is logged to stderr, but the exit code is misleading.

**Severity**: Low — defensive design tradeoff; acceptable for production robustness but could hide bugs.

### M-3 (Low): `cancelled` status inconsistency between publisher and subscriber

**Location**: `publish_event.py:49` vs `job_subscriber.py:46`

- `publish_event.py`: `TERMINAL_EVENTS = ("completed", "error", "cancelled")` — publishes `cancelled` with retain=True
- `job_subscriber.py`: `TERMINAL_EVENTS = ("completed", "error")` — does NOT treat `cancelled` as terminal in the MQTT event path (line 292)

If a `cancelled` event arrives via MQTT, the subscriber ignores it as non-terminal and waits until timeout. The disk fallback in `_check_disk_fallback` does handle `cancelled` (maps to `error`), creating an inconsistency between the two paths.

**Note**: This is a pre-existing inconsistency, not introduced by this change. The disk fallback's handling of `cancelled` is an improvement, but the MQTT event path remains incomplete.

**Severity**: Low — pre-existing; `cancelled` events are rare in the current workflow.

### M-4 (Low): Resource leak if `loop_start()` fails after successful `connect()`

**Location**: `job_subscriber.py:237-243`

If `client.connect()` succeeds but `client.loop_start()` raises, the exception is caught, `connected` stays `False`, and the `finally` block skips `client.disconnect()`. The TCP socket may remain open.

**Severity**: Low — `loop_start()` very rarely fails in practice.

### M-5 (Low): `_format_line` potential TypeError if `event` key is present but `None`

**Location**: `job_subscriber.py:55`

`payload.get('event', '?') + source_tag` — if the `event` key exists with value `None`, `None + str` raises `TypeError`. The default `'?'` only applies when the key is **absent**, not when it's `None`.

**Severity**: Very Low — event payloads always have string event fields in practice.

---

## 4. Codebase Accuracy Verification

| Claim in implementation_plan.md / code | Actual | Match |
|-----------------------------------------|--------|-------|
| "multi-agent-mux-delegate-job:331-341" for rc=3 mapping | `elif [[ $sub_rc -eq 3 ]]` at line 340, `broker_unavailable` at 341 | ✅ |
| `with_retry(...)` called with `()` to invoke wrapper | Confirmed: `with_retry(lambda: client.connect(...), ...)()` | ✅ |
| `read_logged_status` returns a status string | Returns `Optional[Dict]` — **mismatch** (see M-1) | ❌ |
| `update_job_status` mirrors to status.json | Confirmed: calls `update_logged_status()` at mqtt_common.py:395 | ✅ |
| 280 → 290 tests | 290 collected (was 280 before +10 new) | ✅ |
| G-1~G-10 all pass | 10/10 PASSED | ✅ |
| M1 checkboxes `[ ]` → `[x]` | All 4 M1 lines updated correctly | ✅ |

---

## 5. Test Quality Assessment

### 5.1 Guard Test Assertion Strength

| Guard | Assertion | Mutation Detection |
|-------|-----------|-------------------|
| G-1 | `rc == 2` + `loaded["status"] == "completed"` | Strong: catches if `return 2` moved before status sync |
| G-2 | `published is False` + `publish_error is not None` | Strong: catches if audit fields omitted on failure |
| G-3 | `published is True` + `publish_error is None` | Strong: catches if success path doesn't set fields |
| G-4 | `last_seq == 1` then `== 2` | Strong: catches if seq not consumed on failure |
| G-5 | `rc == 0` on broker down + disk completed | Strong: catches if disk fallback missing |
| G-6 | `"disk-fallback" in captured.out` | Strong: catches if source tag omitted |
| G-7 | `rc == 1` on disk error | Strong: catches if error status not mapped to rc=1 |
| G-8 | `rc == 2` with partial pending | Strong: catches early-exit bug in wait-any |
| G-9 | `rc == 3` on broker down without disk terminal | Strong: catches if rc=3 not returned |
| G-10 | Static string assertions on script content | Moderate: structural only, not behavioral |

### 5.2 Test Gaps

- **No test for M-1**: No test exercises the `read_logged_status()` fallback path (status.json without registry JSON). A test that deletes the registry JSON but leaves status.json would expose the bug.
- **G-10 is structural**: Only checks string presence in the script, doesn't test runtime behavior of rc=3 mapping. However, G-9 covers the subscriber side behaviorally.
- **No mutation testing run**: The plan claims "100% mutation detection" but no mutation testing tool (e.g., mutmut, cosmic-ray) was run. The claim is based on assertion strength analysis, not empirical verification.

---

## 6. Cross-Document Consistency

- `implementation_plan.md` M1 checkboxes: ✅ All 4 steps marked `[x]`
- Plan references `B-14`, `B-15`, `F-4`, `G-1~G-10` — all present in code/tests
- Plan line 39: "G-1 ~ G-10 가드 통과 + mutation 전건 FAIL 확인 (280 -> 290)" — test count matches (290); mutation testing not empirically verified
- `TERMINAL_EVENTS` mismatch between publish_event.py and job_subscriber.py (M-3) is pre-existing and not addressed in M1 scope

---

## 7. Verdict

The M1/Track 0 implementation correctly addresses all four steps:
1. ✅ B-14: `publish_event.py` performs disk persistence (audit log + registry status) before returning rc=2 on publish failure
2. ✅ B-15: `job_subscriber.py` polls disk every 3s, resolves terminal states via registry JSON fallback, and exits cleanly
3. ✅ F-4: `multi-agent-mux-delegate-job` maps rc=3 to `broker_unavailable` with disk status recheck
4. ✅ G-1~G-10: 10 regression guards implemented, all pass; 290 tests collected

The primary functionality is correct and all tests pass. Five minor findings (M-1~M-5) were identified, with M-1 being the most significant (status.json fallback is dead code due to type mismatch). M-1 is a one-line fix that does not break the primary disk fallback path. None of the findings require design-level rework or replanning.

[VERDICT: PASS]