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:

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.
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).
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.
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).
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).
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]

9.2 KiB Raw Blame History Unescape Escape