release(v4.7.3): bump framework and eight skills to 4.7.3 in lockstep; archive PR #37 merge-side reviews
Issue #36: delegated prompts travel by file, a delegation that fails ends the loop with rc 1. VERSIONS.md carries the entry; the merge-side reports (round 1 @59be1c8 and round 2 @708262a with identity sidecars, base59be1c8-> head708262a, evidence review-tree) are archived under .agents/reports/<session>/. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
GodopuandClaude Fable 5.1 committed
1 parent
708262a598
commit
c3e6a7e96d
28 files changed
+740
-26
No files matched your search
@@ -0,0 +1,14 @@
|
||||
{
|
||||
"loop_id": "loop-20260924T085626Z-b8987e",
|
||||
"base_sha": "59be1c89209a232059aec0c5aeecc2329929cb49",
|
||||
"head_sha": "708262a598c4301c0ccc70efba5401093b4b1e30",
|
||||
"tree_sha": "d601c2d01bb4c09931e5e1989e140b01d51f5b96",
|
||||
"review_commit": "708262a598c4301c0ccc70efba5401093b4b1e30",
|
||||
"reviewer": "reviewer-agy-01",
|
||||
"job_id": "3121d389",
|
||||
"verdict": "PASS",
|
||||
"transport": "herdr",
|
||||
"evidence": "review-tree",
|
||||
"isolation": "worktree",
|
||||
"verified_at": "2026-09-24T09:09:43Z"
|
||||
}
|
||||
@@ -0,0 +1,76 @@
|
||||
# Pre-Merge Independent Delta Review Report: PR #37 (`fix/issue-36` Round 2)
|
||||
|
||||
- **Job ID**: `3121d389`
|
||||
- **Target Agent**: `agy` (Session: `herdr:reviewer-agy-01`)
|
||||
- **Role**: Reviewer
|
||||
- **Target Worktree Context**:
|
||||
- **Loop ID**: `loop-20260924T085626Z-b8987e`
|
||||
- **Base Commit**: `59be1c89209a232059aec0c5aeecc2329929cb49` (Round 1 target)
|
||||
- **Head Commit**: `708262a598c4301c0ccc70efba5401093b4b1e30` (1 delta commit: +68 / −7)
|
||||
- **Tree SHA**: `d601c2d01bb4c09931e5e1989e140b01d51f5b96`
|
||||
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20260924T085626Z-b8987e/1`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
This second review round examines delta commit `708262a` ("fix(issue-36): merge review - handle prompt write failure in runner; reject empty prompt file; make test_s9 stderr check portable").
|
||||
|
||||
The commit directly resolves all three findings identified during Round 1:
|
||||
1. **R1 / H1 (Prompt Write Failure Handling in `runner.py`)**: `_run_cmd` now wraps temporary prompt file creation and writing in `try ... except OSError:`. If writing fails (e.g. `ENOSPC`, `EFBIG`), any partial prompt file is unlinked and a `CompletedProcess(..., 126, "", msg)` is returned. The wrapper is not invoked, no unhandled exceptions bubble to `LoopRunner.run()`, and the failure is logged cleanly. Verified by new test `test_s4c`.
|
||||
2. **R2 / N1 (macOS Portability in `test_s9`)**: In `tests/test_issue36_prompt_transport.py`, line 637 guards the assertion `assert "File too large" in result.stderr` with `if sys.platform == "linux":`. This accommodates macOS `/bin/bash` 3.2.57 behavior while continuing to assert `result.returncode != 0`, clean file removal (`assert not leftovers`), and the portable MAM diagnostic (`assert "delegate_job_safe: cannot write the prompt file" in result.stderr`) across all platforms.
|
||||
3. **H3 (Empty Prompt File Rejection)**: `multi-agent-mux-delegate-job` now asserts `[[ -s "$PROMPT_FILE" ]]` in `cmd_submit()`, rejecting empty files before registration. Verified by new test `test_s1e`.
|
||||
|
||||
All 23 applicable tests in `tests/test_issue36_prompt_transport.py` passed cleanly in the macOS verification environment (with 1 platform-specific test skipped on Darwin as intended).
|
||||
|
||||
---
|
||||
|
||||
## 2. Detailed Verification Against Checklist Items
|
||||
|
||||
### (1) Verification of the Three Delta Fixes & Side-Effect Analysis
|
||||
- **Prompt File Creation & Cleanup in `runner.py` (R1/H1)**:
|
||||
- `prompt_file` is initialized to `""`.
|
||||
- In `_run_cmd`, `tempfile.NamedTemporaryFile` and `fh.write(p_text)` are wrapped in `try ... except OSError as e:`.
|
||||
- If writing fails:
|
||||
* If `prompt_file` exists, it is unlinked immediately in the `except` block.
|
||||
* Returns `subprocess.CompletedProcess(["bash", str(wrapper_script), "submit"], 126, "", msg)`.
|
||||
* Because it returns directly from the `except` block, the downstream `subprocess.run(cmd)` and the outer `finally:` block do not execute. The wrapper is never invoked with an incomplete file.
|
||||
- On the normal path:
|
||||
* The wrapper executes, and the outer `finally: os.unlink(prompt_file)` ensures the file is deleted exactly once.
|
||||
- The returned exit code 126 triggers the existing retry mechanism in `delegate_job()`; if the write still fails, the error message is logged cleanly and `delegate_job()` returns `None`, terminating the loop with exit code 1 without tracebacks.
|
||||
- Verified by `test_s4c_a_prompt_file_that_cannot_be_written_is_a_failed_submit`.
|
||||
- **macOS Test Portability for `test_s9` (R2/N1)**:
|
||||
- Scoping `assert "File too large" in result.stderr` to `sys.platform == "linux"` eliminates the macOS `AssertionError` that caused the Round 1 `NOT PASS`.
|
||||
- Portable assertions across Darwin and Linux verify:
|
||||
* `assert result.returncode != 0` (non-zero exit upon write failure).
|
||||
* `assert not leftovers` (partial `.mam/prompt-shloop-*` files are deleted).
|
||||
* `assert "delegate_job_safe: cannot write the prompt file" in result.stderr` (MAM error diagnostics printed to stderr).
|
||||
- Verified by running `tests/test_issue36_prompt_transport.py::test_s9_shell_loop_removes_a_partial_prompt_file_and_says_why` on macOS (PASS).
|
||||
- **Empty Prompt File Rejection (H3)**:
|
||||
- `cmd_submit()` in `multi-agent-mux-delegate-job` checks `[[ -s "$PROMPT_FILE" ]] || { echo "submit: --prompt-file $PROMPT_FILE is empty" >&2; exit 1; }`.
|
||||
- Rejects zero-byte prompt files fail-closed before invoking `registry.py`.
|
||||
- Verified by `test_s1e_an_empty_prompt_file_is_refused`.
|
||||
|
||||
### (2) Impact of Empty File Rejection on Valid Workflows
|
||||
- In MAM, all valid delegated prompts (task instructions, review requests, refinement briefs, compliance orders) require meaningful textual context.
|
||||
- Loop brief provisioning (`prompt.md`) always writes non-empty markdown briefs containing metadata and task descriptions.
|
||||
- Enforcing `-s` prevents unintended empty job dispatches without affecting legitimate system workflows.
|
||||
|
||||
### (3) Overall Merge Readiness
|
||||
- Commit `708262a` cleanly addresses all review concerns with minimal, surgical changes (+68 / −7).
|
||||
- `git diff 59be1c8...HEAD --check` reported 0 whitespace or formatting errors.
|
||||
- Version bump to `v4.7.3` is scheduled for the orchestrator release commit.
|
||||
- Zero blockers remaining.
|
||||
|
||||
---
|
||||
|
||||
## 3. Test Execution Summary
|
||||
|
||||
| Test Suite / Target | Result | Duration | Notes |
|
||||
| :--- | :--- | :--- | :--- |
|
||||
| `tests/test_issue36_prompt_transport.py` | **23 PASS, 1 SKIP** | 42.4s | 23 passed on macOS; `test_s1b` skipped (`linux_only` constant test); `test_s9`, `test_s4c`, `test_s1e` all pass |
|
||||
| `git diff 59be1c8...HEAD --check` | **0 errors** | 0.05s | Clean formatting and no whitespace errors |
|
||||
|
||||
---
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,135 @@
|
||||
# Pre-Merge Independent Review Report: PR #37 (`fix/issue-36`)
|
||||
|
||||
- **Job ID**: `db46a0da`
|
||||
- **Target Agent**: `agy` (Session: `herdr:reviewer-agy-01`)
|
||||
- **Role**: Reviewer
|
||||
- **Target Worktree Context**:
|
||||
- **Loop ID**: `loop-20260924T080014Z-2b3df0`
|
||||
- **Base Commit**: `1debc7d92444385b9200e821efa0ff0f6d82023e` (v4.7.2)
|
||||
- **Head Commit**: `59be1c89209a232059aec0c5aeecc2329929cb49` (6 commits: +765 / −30)
|
||||
- **Tree SHA**: `a009c9062d9818f343ac57e6c72cda932887fab7`
|
||||
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20260924T080014Z-2b3df0/1`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
PR #37 resolves Issue #36 ("위임 프롬프트를 argv 가 아닌 파일로 전달하고, 위임 실패를 종료 코드 1 로 보고", Closes #36) across 6 commits and 11 files (+765 / −30):
|
||||
- `59be1c8` fix(issue-36): create_session hands the agent a brief path, not the task; a prompt file that cannot be written is removed
|
||||
- `f26952c fix(issue-36): the legacy shell loop and lib.sh's submit helpers pass prompts by file
|
||||
- `14d11bf` fix(issue-36): a job that cannot be registered after a rebuttal ends the loop with rc 1
|
||||
- `f5cadd9` test(issue-36): prompts over the argv cap go through the real wrapper, registry and loop; a failed delegation ends the loop with rc 1 and no traceback
|
||||
- `c77806b` fix(issue-36): the loop passes every delegated prompt as a file; a wrapper that cannot start is a failed submit, on the first try and on the retry
|
||||
- `68a5d31` fix(issue-36): registry and wrapper accept --prompt-file; the wrapper hands the same path on
|
||||
|
||||
The core architecture correctly eliminates the 131,072-byte `MAX_ARG_STRLEN` barrier on Linux by passing prompts via temporary files (`--prompt-file`) across the Python loop runner, delegate-job wrapper, `registry.py`, `lib.sh`, and `create_session.sh`. All legacy CLI contracts and `--prompt` backward compatibility remain intact.
|
||||
|
||||
However, during execution in the verification environment on macOS, test `test_s9_shell_loop_removes_a_partial_prompt_file_and_says_why` in `tests/test_issue36_prompt_transport.py` **failed** with an `AssertionError` at line 602 (`assert "File too large" in result.stderr`). This is a platform-dependent test expectation: GNU bash / glibc on Linux prints `write error: File too large` when `ulimit -f` is exceeded, whereas macOS `/bin/bash` (3.2.57) does not emit that string. Because the test suite must pass cleanly across supported platforms (macOS/Linux), a verdict of `NOT PASS` is issued.
|
||||
|
||||
---
|
||||
|
||||
## 2. Blocking Defect Details
|
||||
|
||||
### [Defect 1] Platform-Specific Assertion Failure on macOS in `test_s9`
|
||||
- **File & Line**: `tests/test_issue36_prompt_transport.py:602`
|
||||
- **Code**:
|
||||
```python
|
||||
598: harness.write_text('set -euo pipefail\nREPO_ROOT="$PWD"\nMAM_REAL_ROOT="$PWD"\n'
|
||||
599: 'log_error() { printf "%s\\n" "$1"; }\n' + function +
|
||||
600: '\ntrap "" XFSZ\nulimit -f 1\n'
|
||||
601: 'output=$(delegate_job_safe submit --prompt "$(cat input.md)")\n')
|
||||
602: result = subprocess.run(["bash", str(harness)], cwd=tmp_path, capture_output=True, text=True, timeout=60)
|
||||
603: assert result.returncode != 0
|
||||
604: assert "File too large" in result.stderr
|
||||
605: leftovers = list((tmp_path / ".mam").glob("prompt-shloop-*"))
|
||||
606: assert not leftovers, [(p.name, p.stat().st_size) for p in leftovers]
|
||||
607: assert "delegate_job_safe: cannot write the prompt file" in result.stderr
|
||||
```
|
||||
- **Error Trace**:
|
||||
```
|
||||
FAILED tests/test_issue36_prompt_transport.py::test_s9_shell_loop_removes_a_partial_prompt_file_and_says_why
|
||||
AssertionError: assert 'File too large' in 'xxxx...xxxxdelegate_job_safe: cannot write the prompt file under /private/tmp/.../.mam\n'
|
||||
```
|
||||
- **Root Cause**:
|
||||
When `ulimit -f 1` is exceeded during `printf '%s' "$2" > "$prompt_file"`:
|
||||
* On Linux (GNU bash 5.x / glibc), the shell writes `bash: line X: printf: write error: File too large` to stderr.
|
||||
* On macOS (Darwin / bash 3.2.57), the redirection write fails and `printf` exits non-zero without printing the exact literal `"File too large"`.
|
||||
* The actual implementation behavior is completely correct on macOS: `result.returncode != 0` (Line 603 passes), partial files are cleaned up (`assert not leftovers` Line 606 passes), and MAM's error message `delegate_job_safe: cannot write the prompt file` is printed to stderr (Line 607 passes).
|
||||
* Line 604 strictly demands `"File too large"` unconditionally, causing the test to fail on macOS.
|
||||
- **Remediation**:
|
||||
Guard the assertion with `sys.platform == "linux"`, or allow MAM's error string to satisfy the failure check:
|
||||
```python
|
||||
if sys.platform == "linux":
|
||||
assert "File too large" in result.stderr
|
||||
```
|
||||
or
|
||||
```python
|
||||
assert ("File too large" in result.stderr or "cannot write the prompt file" in result.stderr)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 3. Detailed Verification of Other Checklist Items
|
||||
|
||||
### (1) Completeness of Prompt Transport Pathways
|
||||
- **No Residual Raw `--prompt` Exec Invocations**:
|
||||
- `lib_py/loop/runner.py`: `_run_cmd` creates a temporary file under `self.mam_dir` (`prompt-{loop_id}-*.md`) and invokes the wrapper with `--prompt-file <path>`. The temporary file is safely cleaned up in a `finally:` block.
|
||||
- `lib_py/delegation/registry.py`: Supports mutually exclusive `--prompt` and `--prompt-file` in both `register` and `update` commands. Evaluated via `_prompt_from_args()` during CLI parsing before writing to registry JSON.
|
||||
- `lib_py/delegation/cli.py`: Supports `--prompt-file`, forwarding to `registry.py`.
|
||||
- `lib.sh`: `delegate_submit_job()` and `_delegate_submit_job_legacy()` both utilize `_delegate_prompt_file` and clean up via `rm -f "$prompt_file"`.
|
||||
- `multi-agent-mux-delegate-job`: Iteration brief provisions write to `$job_dir/prompt.md` and invoke `registry.py update --prompt-file "$job_dir/prompt.md"`.
|
||||
- `run_loop.sh`: `delegate_job_safe` intercepts `--prompt` and converts it into `--prompt-file "$prompt_file"`, unlinking it upon return.
|
||||
- `create_session.sh`: Injected pane text no longer includes the full prompt text; instead, writes `$SUBMIT_JOB_PROMPT` to `$task_brief` (`.mam/jobs/<id>/brief.md`) and passes: `Task: read the complete task in $task_brief and follow it.`.
|
||||
- Relative paths passed to `--prompt-file` in `cmd_submit` are converted to absolute paths against caller `pwd` before changing directory to `WORKDIR` (`multi-agent-mux-delegate-job:384`), preventing directory-shift bugs.
|
||||
|
||||
### (2) Error and Failure Handling
|
||||
- **`OSError` Handling on Wrapper Execution**:
|
||||
- In `runner.py`:
|
||||
```python
|
||||
except OSError as e:
|
||||
msg = (f"cannot start the wrapper: {e} "
|
||||
f"(prompt {len(p_text.encode('utf-8'))} bytes, wrapper {wrapper_script})")
|
||||
return subprocess.CompletedProcess(cmd, 126, "", msg)
|
||||
```
|
||||
Wraps execution failure cleanly as returncode 126. The loop logs the failure and aborts with rc 1 without tracebacks (`test_s3b`, `test_s4`).
|
||||
- **Fail-Closed Unregistered / Failed Jobs**:
|
||||
- Re-adjudication failure (`if not adj_jid:`): terminates with error and rc 1.
|
||||
- Arbitration failure (`if not arb_jid:`): terminates with error and rc 1.
|
||||
- Compliance failure (`if not comp_jid or not self.wait_for_job(comp_jid):`): terminates with error and rc 1.
|
||||
- Changing compliance failure from continue to `return 1` is an intentional, sound hardening: if the compliance job fails or times out after an overruled rebuttal, the Creator did not implement the required changes; proceeding to review un-updated code would be erroneous.
|
||||
|
||||
### (3) Evidence vs. Delivery Sizing (`MAM_DIFF_MAX_BYTES` / `MAM_DIFF_MAX_LINES`)
|
||||
- In `diff_collect.sh`: Comments clarify that `MAM_DIFF_MAX_BYTES` (default 200,000) and `MAM_DIFF_MAX_LINES` (default 4000) bound the evidence shown to reviewers, while delivery itself is unbound due to `--prompt-file`.
|
||||
- `test_s5_a_diff_in_the_former_blind_spot_is_delivered_whole` confirms a 150,000-byte diff (between 131,072 and 200,000 bytes) is not truncated and delivered intact.
|
||||
|
||||
### (4) `create_session.sh` Onboarding & Brief Alignment
|
||||
- Task text is written to `.mam/jobs/<id>/brief.md`.
|
||||
- Injected instruction provides the absolute path to `brief.md`.
|
||||
- Handshake protocol (`delegate_publish_cmd`, `--event started`, token verification) remains 100% compliant with Issue #23. Verified by `test_issue23_24_startup_path.py` (19/19 tests passed).
|
||||
|
||||
### (5) Existing Test Contracts & Backwards Compatibility
|
||||
- `test_issue5_phase1_delegation.py`: **66 / 66 passed**.
|
||||
- `test_version_consistency.py`: **3 / 3 passed**.
|
||||
- `test_loop_cli.py`: **9 / 9 passed**.
|
||||
- `test_issue14_delivery_ack.py`: **12 / 12 passed**.
|
||||
- `test_issue21_phase2_loop_a.py`: **29 / 29 passed**.
|
||||
- Existing `--prompt` syntax remains fully supported for backward compatibility across all CLIs.
|
||||
|
||||
---
|
||||
|
||||
## 4. Test Execution Summary
|
||||
|
||||
| Test Suite | Result | Duration | Notes |
|
||||
| :--- | :--- | :--- | :--- |
|
||||
| `tests/test_issue36_prompt_transport.py` | **20 PASS, 1 FAIL, 1 SKIP** | 39.9s | `test_s9` failed on line 602 (`File too large` assertion on macOS); `test_s1b` skipped (`linux_only`) |
|
||||
| `tests/test_issue5_phase1_delegation.py` | **66 / 66 PASS** | 12.1s | Registry CLI, backward compatibility with `--prompt` |
|
||||
| `tests/test_issue23_24_startup_path.py` (S7/S8) | **19 / 19 PASS** | 63.1s | `create_session.sh` brief task injection & handshake |
|
||||
| `tests/test_loop_cli.py` | **9 / 9 PASS** | 39.2s | Loop CLI regressions |
|
||||
| `tests/test_issue14_delivery_ack.py` | **12 / 12 PASS** | 98.2s | Delivery acknowledgement contracts |
|
||||
| `tests/test_issue21_phase2_loop_a.py` | **29 / 29 PASS** | 155.3s | Isolated worktree loop review regressions |
|
||||
| `tests/test_version_consistency.py` | **3 / 3 PASS** | 0.3s | Version consistency checks |
|
||||
| `git diff 1debc7d...HEAD --check` | **0 errors** | 0.05s | Clean formatting and no whitespace errors |
|
||||
|
||||
---
|
||||
|
||||
[VERDICT: NOT PASS]
|
||||
Reference in new issue
Block a user