fix(loop): resolve B-13 by implementing runtime freeze snapshot and dual-root isolation
- Add Stage 2 runtime freeze snapshot at run_loop.sh bootstrap to prevent in-flight tooling mutations - Implement dual-root architecture separating code execution (frozen snapshot) and workspace state (real repo) - Ensure original argv preservation and safe cleanup of freeze directories in exit traps - Add 5 regression guards in tests/test_o3_scoped_guard.py (271/271 PASS) - Update IMPROVEMENTS.md, VERSIONS.md, and include peer review report
This commit is contained in:
@@ -0,0 +1,159 @@
|
||||
# Cross-Code Review Report: B-13 Stage 2 — Runtime Freeze Snapshot
|
||||
|
||||
- **Job ID**: 86163ca6
|
||||
- **Reviewer**: cline
|
||||
- **Date**: 2026-08-17
|
||||
- **Scope**: B-13 Stage 2 runtime freeze snapshot in `run_loop.sh`, 5 new regression tests in `tests/test_o3_scoped_guard.py`, and documentation updates in `IMPROVEMENTS.md` and `VERSIONS.md`
|
||||
|
||||
---
|
||||
|
||||
## 1. Changeset Overview
|
||||
|
||||
| File | Lines Changed | Description |
|
||||
|---|---|---|
|
||||
| `.agents/skills/multi-agent-mux-loop/scripts/run_loop.sh` | +44/-27 | Freeze snapshot logic, argv capture, `MAM_REAL_ROOT` separation, extended `_mam_release_guard` |
|
||||
| `tests/test_o3_scoped_guard.py` | +102/-0 | 5 new B-13 regression tests |
|
||||
| `IMPROVEMENTS.md` | +16/-7 | B-13 moved from open to completed; 271/271 test count |
|
||||
| `VERSIONS.md` | +9/-0 | New entry #8 for B-13/Stage 2 |
|
||||
| **Total** | **+174/-31** | 4 files |
|
||||
|
||||
---
|
||||
|
||||
## 2. Lint Perspective
|
||||
|
||||
### 2.1 Bash Syntax (`run_loop.sh`)
|
||||
- `bash -n run_loop.sh` → **PASS** (no syntax errors)
|
||||
- `shellcheck` not available on this system; manual review performed
|
||||
|
||||
### 2.2 Bash 3.2 Compatibility
|
||||
- `${MAM_LOOP_ARGV[@]+"${MAM_LOOP_ARGV[@]}"}` (line 108): Valid bash 3.2 guard for expanding potentially empty arrays. Without this guard, bash 3.2 (macOS default) would error on `"${MAM_LOOP_ARGV[@]}"` when the array is empty. **Correct.**
|
||||
|
||||
### 2.3 Python Compilation (`test_o3_scoped_guard.py`)
|
||||
- `py_compile test_o3_scoped_guard.py` → **PASS** (no compile errors)
|
||||
- All imports (`os`, `json`, `shutil`, `subprocess`, `time`, `Path`, `pytest`) are used; no unused imports introduced
|
||||
|
||||
### 2.4 Code Style
|
||||
- Variable naming (`MAM_LOOP_ARGV`, `MAM_REAL_ROOT`, `MAM_LOOP_FREEZE_DIR`, `MAM_LOOP_FREEZE_OWNED`, `MAM_LOOP_NO_FREEZE`) follows existing `MAM_*` convention
|
||||
- Comment style matches existing patterns (Korean/English mixed, inline references to bug IDs)
|
||||
- No trailing whitespace or formatting issues introduced
|
||||
|
||||
**Lint Verdict: PASS**
|
||||
|
||||
---
|
||||
|
||||
## 3. Operability Perspective
|
||||
|
||||
### 3.1 Full Test Suite
|
||||
- **271 passed in 445.76s (0:07:25)** — EXIT_CODE:0
|
||||
- Previous baseline: 266 tests (job 2f64681f). New total: 266 + 5 B-13 tests = 271. **Consistent.**
|
||||
- Test count in IMPROVEMENTS.md (271/271) and VERSIONS.md (271/271) matches actual results.
|
||||
|
||||
### 3.2 B-13 Regression Tests (5/5 PASS in 0.34s)
|
||||
|
||||
| Test | Status | What It Verifies |
|
||||
|---|---|---|
|
||||
| `test_b13_reexec_preserves_original_argv` | PASS | Argv forwarded through freeze re-exec (arg parser consumes `$@` via shift) |
|
||||
| `test_b13_freeze_survives_broken_wrapper` | PASS | Frozen copy immune to wrapper broken mid-loop |
|
||||
| `test_b13_freeze_dir_is_outside_the_skill_tree` | PASS | No files written under `.agents/skills/` (B-6 boundary) |
|
||||
| `test_b13_release_guard_cleans_up_and_releases_lock` | PASS | Extended `_mam_release_guard` releases lock + removes snapshot |
|
||||
| `test_b13_no_freeze_switch_disables_reexec` | PASS | `MAM_LOOP_NO_FREEZE=1` skips freeze entirely |
|
||||
|
||||
### 3.3 Existing Test Regression Check
|
||||
- All 27 pre-existing tests in `test_o3_scoped_guard.py` still pass (32/32 total in file)
|
||||
- No regressions detected in any test file
|
||||
|
||||
### 3.4 Live Freeze Verification
|
||||
- During this review, the actual `run_loop.sh` orchestrator (PID 88241) was observed running from `/var/folders/.../mam-loop-freeze.L2nD67/.agents/skills/multi-agent-mux-loop/scripts/run_loop.sh` — confirming the freeze mechanism works in production, not just in tests.
|
||||
|
||||
### 3.5 Freeze Logic Analysis
|
||||
|
||||
**Code path vs. Data path separation:**
|
||||
|
||||
| Variable | After Re-exec | Used For | Correct? |
|
||||
|---|---|---|---|
|
||||
| `$REPO_ROOT` | Freeze dir (e.g., `/tmp/mam-loop-freeze.XXXXXX`) | Loading scripts (`source`), running wrapper (`delegate_job_safe`) | Yes — code runs from frozen snapshot |
|
||||
| `$MAM_REAL_ROOT` | Original repo (e.g., `/Users/.../multi-agent-mux`) | Lock marker, `.tmp` cleanup, `git rev-parse`, `mam_collect_changes_diff` | Yes — state/git ops use real repo |
|
||||
|
||||
**3 `$REPO_ROOT` → `$MAM_REAL_ROOT` conversions** (lines 175, 488, 580):
|
||||
- Line 175: `rm -f "$MAM_REAL_ROOT/.agents/skills/..."` — cleans `.tmp` files in real repo (was `$REPO_ROOT`)
|
||||
- Line 488: `BASE_COMMIT=$(cd -P "$MAM_REAL_ROOT" ...)` — git operations in real repo (was `$REPO_ROOT`)
|
||||
- Line 580: `mam_collect_changes_diff "$MAM_REAL_ROOT" ...` — diff collection in real repo (was `$REPO_ROOT`)
|
||||
|
||||
All remaining `$REPO_ROOT` usages (lines 17, 19, 21, 101, 104-106, 130) are correct — they either load scripts from the freeze dir or export env vars before the re-exec.
|
||||
|
||||
**Graceful degradation:**
|
||||
- If `mktemp -d` fails or `cp -R` fails, the freeze is aborted, the temp dir is cleaned up, and a warning is printed via `echo` (not `log_warn`, which isn't defined until line 141). The script continues unfrozen. **Correct.**
|
||||
|
||||
**Cleanup safety:**
|
||||
- `_mam_release_guard` only deletes the freeze dir if `MAM_LOOP_FREEZE_OWNED=1` (set by the freeze creator)
|
||||
- `case "$MAM_LOOP_FREEZE_DIR" in */mam-loop-freeze.*) rm -rf ...` — pattern guard prevents accidental deletion of arbitrary directories. **Safe.**
|
||||
|
||||
**Operability Verdict: PASS**
|
||||
|
||||
---
|
||||
|
||||
## 4. Loss Perspective
|
||||
|
||||
### 4.1 Behaviors Preserved
|
||||
- Lock acquisition/release mechanism unchanged (`mam_acquire_loop_lock` / `mam_release_loop_lock`)
|
||||
- `delegate_job_safe` still runs wrapper from `$REPO_ROOT` (which is now the freeze dir — correct)
|
||||
- `--all-reviewer`, `--max-loop`, `--verbose` etc. all work the same (argv preserved through re-exec)
|
||||
- `.mam.env` loading preserved via `MAM_ENV_FILE` export (wrapper checks `MAM_ENV_FILE` first)
|
||||
|
||||
### 4.2 Behaviors Changed (Intentional)
|
||||
- `run_loop.sh` now re-execs from a frozen snapshot at startup (by default)
|
||||
- `_mam_release_guard` extended with freeze dir cleanup (additive — lock release still works)
|
||||
- 3 git/state operations switched from `$REPO_ROOT` to `$MAM_REAL_ROOT` (necessary after freeze)
|
||||
- `MAM_LOOP_NO_FREEZE=1` opt-out switch added (for testing/debugging)
|
||||
|
||||
### 4.3 No Unintended Losses
|
||||
- No functions removed or renamed (only `orig_script` → `wrapper_script` cosmetic rename in `delegate_job_safe`)
|
||||
- No environment variables removed
|
||||
- No existing test modified or removed
|
||||
- Old comment about "deliberately creates no copy" replaced with accurate description of freeze mechanism
|
||||
|
||||
**Loss Verdict: PASS**
|
||||
|
||||
---
|
||||
|
||||
## 5. Documentation Review
|
||||
|
||||
### 5.1 IMPROVEMENTS.md
|
||||
- B-13 moved from "Section 2: Edge-case Bugs" (open) to "Section 5: Completed Tasks" (completed)
|
||||
- Open task count: 3 → 2 (correct)
|
||||
- Completed task count: 22 → 23 (correct, B-13 added)
|
||||
- B-13 completion entry includes: freeze mechanism, B-13 layer (bash byte-offset), B-6 distinction, P1/C1/C2 corrections, cleanup logic, 5 regression guards
|
||||
- Priority table updated with B-13 entry
|
||||
- Test count updated to 271/271
|
||||
|
||||
### 5.2 VERSIONS.md
|
||||
- New entry #8 added under current version section
|
||||
- Covers: freeze mechanism, code/state root separation, argv preservation, `echo` fallback, `MAM_LOOP_NO_FREEZE=1`, 5 regression guards, 271/271 PASS
|
||||
- Accurate and comprehensive
|
||||
|
||||
### 5.3 Test Name Consistency
|
||||
- All 5 test names in IMPROVEMENTS.md match actual code exactly. **No discrepancies.**
|
||||
|
||||
**Documentation Verdict: PASS**
|
||||
|
||||
---
|
||||
|
||||
## 6. Edge Cases & Safety Analysis
|
||||
|
||||
| Scenario | Handling | Risk |
|
||||
|---|---|---|
|
||||
| SIGKILL during loop | Trap doesn't fire; freeze dir leaks in `$TMPDIR` | Low — OS cleans `$TMPDIR` on reboot; no source tree pollution |
|
||||
| Concurrent loops | Each gets unique `mktemp -d` name; loop lock prevents concurrent execution | None |
|
||||
| Freeze copy race | Freeze happens at init before any workers start | None |
|
||||
| `cp -R` with symlinks | Symlinks preserved as-is in freeze | Low — `.agents/skills/` has no external symlinks |
|
||||
| Empty argv (`$#=0`) | `${MAM_LOOP_ARGV[@]+...}` guard handles empty array in bash 3.2 | None |
|
||||
| `mktemp` failure | `_freeze=""`, falls through to warning + unfrozen continuation | None — graceful degradation |
|
||||
| `.mam.env` absent | `[ -f "$REPO_ROOT/.mam.env" ] && export ...` — short-circuits if absent | None |
|
||||
|
||||
---
|
||||
|
||||
## 7. Summary
|
||||
|
||||
The B-13 Stage 2 implementation correctly addresses the self-hosting loop runtime freeze problem. The freeze snapshot mechanism is sound: it captures `.agents/skills/` into a temp directory at loop initialization and re-execs from the frozen copy, making the running loop immune to mid-loop skill edits. The code path / data path separation (`$REPO_ROOT` for code, `$MAM_REAL_ROOT` for state) is clean and correct. All 271 tests pass, including 5 new B-13 regression tests. No regressions, no losses, no lint issues.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -5,9 +5,14 @@
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
# B-13: the arg parser below consumes "$@" (shift), so capture argv now —
|
||||
# the freeze re-exec needs the original arguments (measured: $#=0 after parse).
|
||||
MAM_LOOP_ARGV=("$@")
|
||||
|
||||
# 1. Load Common Framework Library
|
||||
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
REPO_ROOT="$(cd "$SCRIPT_DIR/../../../.." && pwd)"
|
||||
MAM_REAL_ROOT="${MAM_REAL_ROOT:-$REPO_ROOT}"
|
||||
# shellcheck disable=SC1091
|
||||
source "$REPO_ROOT/.agents/skills/lib.sh"
|
||||
# shellcheck disable=SC1091
|
||||
@@ -84,29 +89,51 @@ if [ -z "$TARGET_AGENT" ] || [ -z "$TASK" ]; then
|
||||
usage
|
||||
fi
|
||||
|
||||
MAM_LOOP_MARKER="${MAM_LOOP_MARKER:-$REPO_ROOT/.mam/loop-guard-active}"
|
||||
_mam_release_guard() { mam_release_loop_lock "$MAM_LOOP_MARKER" || true; }
|
||||
# --- B-13 Stage 2: freeze the runtime before the loop can be edited under us ---
|
||||
# bash keeps reading a running script from disk by byte offset, so a worker that
|
||||
# edits .agents/skills/ mid-loop can break this very file (measured: even a valid
|
||||
# replacement died with "unexpected EOF"). Re-exec once from a snapshot.
|
||||
# NOTE: log_* are not defined until :114 — use echo here, not log_warn (C2).
|
||||
if [ -z "${MAM_LOOP_FREEZE_DIR:-}" ] && [ "${MAM_LOOP_NO_FREEZE:-0}" != "1" ]; then
|
||||
_freeze=""
|
||||
_freeze="$(mktemp -d "${TMPDIR:-/tmp}/mam-loop-freeze.XXXXXX" 2>/dev/null)" || _freeze=""
|
||||
if [ -n "$_freeze" ] && mkdir -p "$_freeze/.agents" 2>/dev/null \
|
||||
&& cp -R "$REPO_ROOT/.agents/skills" "$_freeze/.agents/skills" 2>/dev/null; then
|
||||
export MAM_LOOP_FREEZE_DIR="$_freeze"
|
||||
export MAM_LOOP_FREEZE_OWNED="1" # ← C1: cleanup gate requires this
|
||||
export MAM_REAL_ROOT="$REPO_ROOT"
|
||||
export WORKSPACE_ROOT="$REPO_ROOT"
|
||||
[ -f "$REPO_ROOT/.mam.env" ] && export MAM_ENV_FILE="$REPO_ROOT/.mam.env"
|
||||
exec bash "$_freeze/.agents/skills/multi-agent-mux-loop/scripts/run_loop.sh" \
|
||||
${MAM_LOOP_ARGV[@]+"${MAM_LOOP_ARGV[@]}"} # ← P1: original argv (bash 3.2 guarded)
|
||||
fi
|
||||
[ -n "$_freeze" ] && rm -rf "$_freeze"
|
||||
echo -e "\033[1;33m[!]\033[0m freeze snapshot failed — continuing unfrozen (B-13 protection off)" >&2
|
||||
fi
|
||||
|
||||
# Runs the delegate-job wrapper in place. Deliberately creates no copy and
|
||||
# installs no trap:
|
||||
# * a copy inside .agents/skills/ pollutes the source tree and leaks on
|
||||
# SIGKILL (B-6). It never protected across turns anyway — the copy is made
|
||||
# per call, so a wrapper broken in turn N is copied broken in turn N+1;
|
||||
# * every call site is a command substitution, so a trap set here fires when
|
||||
# that subshell ends. `$$` is still the parent's pid there, so
|
||||
# _mam_release_guard passed its ownership check and dropped the loop lock
|
||||
# after the first delegated job (D1).
|
||||
# The callers' own "Failed to register ..." branches are unreachable when the
|
||||
# wrapper exits non-zero (set -e aborts the assignment first), so the diagnosis
|
||||
# has to be emitted here.
|
||||
MAM_LOOP_MARKER="${MAM_LOOP_MARKER:-$MAM_REAL_ROOT/.mam/loop-guard-active}"
|
||||
_mam_release_guard() {
|
||||
mam_release_loop_lock "$MAM_LOOP_MARKER" || true
|
||||
# B-13: 스냅샷은 우리가 만들었을 때만 지운다 (외부 주입 값은 건드리지 않음)
|
||||
if [ -n "${MAM_LOOP_FREEZE_DIR:-}" ] && [ "${MAM_LOOP_FREEZE_OWNED:-0}" = "1" ]; then
|
||||
case "$MAM_LOOP_FREEZE_DIR" in
|
||||
*/mam-loop-freeze.*) rm -rf "$MAM_LOOP_FREEZE_DIR" ;;
|
||||
*) : ;;
|
||||
esac
|
||||
fi
|
||||
}
|
||||
|
||||
# Runs the delegate-job wrapper from the frozen skills tree (REPO_ROOT).
|
||||
# Stage 2 creates a single snapshot at loop initialization outside the skill tree (B-13),
|
||||
# keeping the running loop immune across turns without per-call copies or tree pollution (B-6).
|
||||
delegate_job_safe() {
|
||||
local orig_script="$REPO_ROOT/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job"
|
||||
local wrapper_script="$REPO_ROOT/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job"
|
||||
local rc=0
|
||||
bash "$orig_script" "$@" || rc=$?
|
||||
bash "$wrapper_script" "$@" || rc=$?
|
||||
if [ "$rc" -ne 0 ]; then
|
||||
log_error "delegate_job_safe failed (exit $rc): $orig_script"
|
||||
log_error "delegate_job_safe failed (exit $rc): $wrapper_script"
|
||||
log_error " if this loop edits framework skills in place, check that file's syntax:"
|
||||
log_error " bash -n \"$orig_script\""
|
||||
log_error " bash -n \"$wrapper_script\""
|
||||
fi
|
||||
return $rc
|
||||
}
|
||||
@@ -145,7 +172,7 @@ case "$_mam_acquire_rc" in
|
||||
;;
|
||||
esac
|
||||
trap _mam_release_guard EXIT INT TERM HUP
|
||||
rm -f "$REPO_ROOT/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job".*.tmp 2>/dev/null || true
|
||||
rm -f "$MAM_REAL_ROOT/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job".*.tmp 2>/dev/null || true
|
||||
|
||||
# --all-reviewer silently takes precedence over an explicit --reviewer list;
|
||||
# warn so the discarded list isn't mistaken for having been honored (P2-1).
|
||||
@@ -458,7 +485,7 @@ log_info "=== Phase 2: Code Implementation ==="
|
||||
|
||||
# Fix the pre-implementation commit as the diff baseline so review diffs stay
|
||||
# cumulative and non-empty even after the Creator commits per DoD (P0-1).
|
||||
BASE_COMMIT=$(cd -P "$REPO_ROOT" 2>/dev/null && git rev-parse HEAD 2>/dev/null || echo "")
|
||||
BASE_COMMIT=$(cd -P "$MAM_REAL_ROOT" 2>/dev/null && git rev-parse HEAD 2>/dev/null || echo "")
|
||||
|
||||
EXECUTION_PROMPT="계획서가 존재하지 않으므로, 작업자(Creator)의 판단하에 스스로 구현 계획 및 설계를 수립한 뒤, 이를 바탕으로 코드를 구현하고 다음 작업 목표를 완성해주세요. 작업 목표: $TASK"
|
||||
if [ -n "$CURRENT_PLAN" ]; then
|
||||
@@ -550,7 +577,7 @@ while [ "$loop_count" -le "$MAX_LOOP" ]; do
|
||||
else
|
||||
log_info "Active reviewers: ${REVIEWERS[*]}"
|
||||
|
||||
CHANGES_DIFF=$(mam_collect_changes_diff "$REPO_ROOT" "$BASE_COMMIT") || {
|
||||
CHANGES_DIFF=$(mam_collect_changes_diff "$MAM_REAL_ROOT" "$BASE_COMMIT") || {
|
||||
log_error "Could not determine the change set; refusing to request a review on no evidence."
|
||||
log_error "$CHANGES_DIFF"
|
||||
exit 1
|
||||
|
||||
Reference in New Issue
Block a user