multi-agent-mux/.agents/reports/canary-projects-multi-agent-mux-creator-cline/report-54413a8a.md

# Cross Code Review Report — Job 54413a8a

- **Job ID**: 54413a8a
- **Reviewer**: cline (session: herdr:canary-projects-multi-agent-mux-creator-cline)
- **Target**: Finalize `.env` → `.mam.env` migration (atomic changes + reviewer validation)
- **Date**: 2026-08-04
- **Diff scope**: 16 files, +300 / −119 (`.gitignore`, `.env.example → .mam.env.example`, `mqtt_common.py`, delegate-job wrapper, `deploy/{generate-env,install,install_mam,remove,update}.sh`, `MULTI_AGENT_RULES.{md,ko.md}`, `BOOTSTRAP.{md,ko.md}`, `README{,.ko}.md`, `deploy/README.md`, `tests/test_env_migration.py`)

> **Iteration context**: This is a follow-up review. The prior review (job `3117bdcc`) found a **BLOCKING** syntax error in `deploy/remove.sh` (orphaned `fi` at line 192 after an `if`→`for` refactor). That defect has been **fixed** in this iteration — `bash -n deploy/remove.sh` now passes and the `for env_name ... done` loop is well-formed.

---

## 1. Summary

The changeset completes the `.env` → `.mam.env` namespace migration with a robust, backward-compatible fallback design:

1. **Runtime loaders** (shell wrapper + `mqtt_common.py`) now prefer `.mam.env`, fall back to `.env` with deprecation warnings, and support an explicit `MAM_ENV_FILE` override.
2. **Boundary-safe workspace resolution** — `mqtt_common._load_dotenv()` walks up from the script location and stops at the first directory containing `.agents/` or `.git/`, preventing parent-directory `.env` leakage (T-4).
3. **Installer migration** — `deploy/install.sh` gains `migrate_legacy_env()` that renames an MAM-owned `.env` → `.mam.env` (evidence-based via install manifest) and rewrites the manifest; unowned `.env` is left untouched with a guidance message.
4. **Uninstaller** — `deploy/remove.sh` now loops over both `.mam.env` and `.env`, with backup deduplication (content-hash via `cmp -s`) and immutable slot-1 retention (T-16/T-17).
5. **Updater** — `deploy/update.sh` pre-captures `MAM_LEGACY_ENV_OWNED`, backs up whichever env file exists, and on restore migrates a legacy-owned `.env` backup forward to `.mam.env` (M-4).
6. **Comprehensive test suite** — `tests/test_env_migration.py` covers 15 scenarios (T-1 → T-17) including precedence, coexistence warnings, boundary stop, OS-env precedence, override, cwd isolation, remove/purge semantics, owned-legacy migration + manifest rewrite, backup dedup, and slot-1 secret retention.
---

## 2. Verification Evidence

### 2.1 Syntax checks — ALL PASS
```
bash -n: 6/6 shell scripts OK
  - .agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job OK
  - deploy/generate-env.sh OK
  - deploy/install.sh OK
  - deploy/install_mam.sh OK
  - deploy/remove.sh OK   ← was FAILING in prior review, now FIXED
  - deploy/update.sh OK
py_compile: mqtt_common.py OK, tests/test_env_migration.py OK
```

### 2.2 Targeted test suite — 15/15 PASS (28.95s)
```
tests/test_env_migration.py::TestEnvMigrationFull
  test_t1_mam_env_only                              PASSED
  test_t2_legacy_env_only_fallback_and_warning      PASSED
  test_t3_coexistence_mam_env_precedence_and_warning PASSED
  test_t4_parent_boundary_stop_walkup_without_arg   PASSED
  test_t5_os_env_precedence                         PASSED
  test_t6_mam_env_file_override                     PASSED
  test_t7_wrapper_cwd_isolation                     PASSED
  test_t8_remove_force_preserves_owned_env          PASSED
  test_t8b_purge_env_is_sole_delete_authority       PASSED
  test_t9_git_check_ignore                          PASSED
  test_t10_shadowing_prevention_guard               PASSED
  test_t12_unowned_legacy_env_preservation          PASSED
  test_t13_owned_legacy_env_migration_and_manifest_rewrite PASSED
  test_t16_backup_deduplication_across_reinstall_cycles PASSED
  test_t17_immutable_slot_1_user_secret_retention   PASSED
============================== 15 passed in 28.95s ==============================
```

### 2.3 `.gitignore` coverage — PASS
```
.mam.env           → ignored (.gitignore:21)
.mam.env.bak       → ignored (.gitignore:22 .mam.env.*)
.mam.env.update-tmp→ ignored (.gitignore:22)
.mam.env.mam-backup→ ignored (.gitignore:22)
.mam.env.example   → NOT ignored (good — negation !.mam.env.example works)
### 2.4 Residual `.env` references — ALL INTENTIONAL
Remaining `.env` references in code are **legacy-fallback / migration-detection** paths, not un-migrated load paths:
- `multi-agent-mux-delegate-job:25-33` — `.env` fallback branches with deprecation warnings (by design).
- `mqtt_common.py:70` — `legacy_env_path = os.path.join(d, ".env")` for fallback + coexistence warning (by design).
- `deploy/install.sh:285-309` — `migrate_legacy_env()` detection of legacy `.env` (by design).
- `deploy/update.sh:71,87-91` — legacy-owned `.env` backup/restore + `MAM_LEGACY_ENV_OWNED` pre-capture (by design).
- `deploy/generate-env.sh:19` — `LEGACY_ENV` for `--migrate-legacy` (by design).
- `BOOTSTRAP.md:128`, `BOOTSTRAP.ko.md:128` — `.gitignore` pattern listing (both `.env` and `.mam.env` patterns retained for the fallback window — correct).

### 2.5 Pre-existing test status (out of scope)
`tests/test_sanity.py::test_create_session_dry_run` FAILS and `test_create_session_full` HANGS — these are **pre-existing** tests (not modified by this change; `git status` shows only `tests/test_env_migration.py` as new). They require a live `herdr`/tmux environment and are unrelated to the env-migration changeset. No regression introduced by this change.

### 2.6 Prior blocking defect — RESOLVED
The orphaned `fi` at line 192 of `deploy/remove.sh` (job `3117bdcc`) is gone. The refactor correctly closes the `for env_name in ".mam.env" ".env"; do ... done` loop (lines 150–206) with no dangling `if/fi` mismatch. Control flow verified by reading lines 148–210.
```
---

## 3. Findings

### 3.1 Blocking defects — NONE

The prior blocking syntax error is resolved. No new blocking defects found.

### 3.2 Non-blocking follow-ups (recommendations)

**R-1 (Low): Stray untracked `.tmp` file persists**
- `?? .agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job.197_38198.tmp` remains in the working tree and is **NOT ignored** by `.gitignore` (`git check-ignore` returns non-zero). This is the same class of issue flagged as R-2 in the prior `OPTIMIZATION.md` review (job `9c44c6b2`).
- **Recommendation**: Add a `*.tmp` rule to `.gitignore` and remove the stray file. Low risk of accidental commit but should be cleaned up.

**R-2 (Low): `mqtt_common._load_dotenv()` boundary check skipped when `workspace_dir` is explicitly provided**
- When `workspace_dir` is passed explicitly (line 66–67), the function uses it directly without verifying a `.agents/` or `.git/` boundary marker. The no-argument path (line 55–65) correctly enforces the boundary. This is acceptable because callers passing an explicit dir are asserting the workspace root, but it is an asymmetry worth a code comment for future maintainers.
- **Recommendation**: Add a one-line comment noting that explicit `workspace_dir` is trusted and bypasses boundary detection. No behavioral change needed.

**R-3 (Low): `deploy/update.sh` legacy-restore branch does not update the install manifest**
- In `update.sh` lines 168–174, when `MAM_LEGACY_ENV_OWNED=1` and `ENV_BACKUP_SRC=".env"`, the backup is restored forward to `.mam.env`. However, unlike `install.sh`'s `migrate_legacy_env()` (which rewrites the manifest `.env`→`.mam.env`), `update.sh` does not rewrite the manifest in this branch. If the manifest still lists `.env`, a subsequent `remove.sh` may not recognize `.mam.env` as MAM-owned.
- **Recommendation**: After the forward-migration `mv` in `update.sh`, also rewrite the manifest entry `.env`→`.mam.env` (mirroring `install.sh`'s python3 one-liner). This is an edge case (legacy-owned env + update without prior install) but could cause `remove.sh` to misclassify `.mam.env` as user-owned on the next uninstall. Low severity because the update path is typically followed by a fresh install that handles manifest rewrite.

---

## 4. Design Assessment

The migration design is **sound and well-layered**:
- **Backward compatibility**: `.env` fallback + deprecation warnings avoid hard breakage for existing users.
- **Evidence-based ownership**: Migration only touches `.env` files the installer can prove it owns (via manifest `grep -Fqx`), preventing accidental takeover of user-owned configs.
- **Data safety**: Backup deduplication (T-16) and immutable slot-1 retention (T-17) prevent both backup proliferation and secret loss across reinstall cycles.
- **Namespace isolation**: Boundary-marker walk-up (T-4) prevents parent-directory `.env` leakage — a real improvement over the prior 5-level blind walk.
- **Testability**: The 15-test suite covers the key edge cases and runs in ~29s without external dependencies.

---

## 5. Verdict

All blocking issues from the prior review are resolved. Syntax checks pass on all 6 shell scripts and 2 Python modules. The targeted regression suite (15/15) passes. `.gitignore` coverage is correct. Residual `.env` references are all intentional fallback/migration paths. The three non-blocking follow-ups (R-1 tmp hygiene, R-2 code comment, R-3 manifest rewrite in update.sh) are low-severity and do not block merge.

[VERDICT: PASS]