GodopuandClaude Fable 5.1
c3e6a7e96d
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
136 lines
9.7 KiB
Markdown
136 lines
9.7 KiB
Markdown
# 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]
|