Second-round reports of the fix/issue-12-8-7 review loop on ea911d7:
planner-reviewer-claude-01 (b368210a), reviewer-agy-01 (3f5d1cd4) and
reviewer-grok-01 (cc059645), all [VERDICT: PASS]. The codex review is
deferred until its usage limit resets; the branch merges only after it.
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
ea911d7842
commit
9d52bb7024
3 files changed
+357
No files matched your search
@@ -0,0 +1,151 @@
|
||||
# Peer Review Report: Branch `fix/issue-12-8-7` (Round 2 — Commits `0fedae2` & `ea911d7`)
|
||||
|
||||
- **Reviewer**: agy (session: `herdr:reviewer-agy-01`)
|
||||
- **Job ID**: `3f5d1cd4`
|
||||
- **Role**: Reviewer
|
||||
- **Target Branch**: `fix/issue-12-8-7`
|
||||
- **Base Commit**: `8a09e02` (v4.4.3)
|
||||
- **Evaluated Commits**:
|
||||
- `e629cc1` — `fix(issue-12,8,7): keep broker password out of records, keep fresh install stamps, pack sibling workspaces`
|
||||
- `4176d75` — `release(v4.4.4): bump framework and eight skills to 4.4.4 in lockstep`
|
||||
- `0fedae2` — `fix(issue-12): scrub-secrets selects only records with a stored password`
|
||||
- `ea911d7` — `fix(issue-8,7): harden install_state merge and pin sibling-workspace edge cases`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
This Round 2 peer review evaluated the complete branch `fix/issue-12-8-7` up to `ea911d7`, focusing specifically on the resolutions introduced in commits `0fedae2` (N1 fix from Claude's Round 1 review: value-based filtering in `scrub-secrets`) and `ea911d7` (non-blocking hardening recommendations for `update.sh` `install_state` merge and two new edge-case tests for Issue #7 workspace packing).
|
||||
|
||||
Key verifications completed:
|
||||
1. **Commit `0fedae2` (N1 Resolution)**: `_cmd_scrub_secrets` now strictly tests `broker.get("password")` (value truthiness) rather than key presence. Unauthenticated records with `"password": null` (~900 in realistic registries) are ignored, preventing spurious rewrite churn and lock contention. Dry-run verified on live `.mam/jobs` (0 records reported).
|
||||
2. **Commit `ea911d7` (Update Hardening)**: `deploy/update.sh` ensures trailing newline on preexisting `install_state`, skips lines missing `=`, and utilizes `awk` with literal variable passing for exact key comparison (`$1 == k`). Failure recovery via `restore_on_failure` is intact.
|
||||
3. **Commit `ea911d7` (Issue #7 Workspace Edge Cases)**: Two new automated tests in `test_issue7_workspace_packing.py` confirm:
|
||||
- An idle pane in a subsequent candidate workspace is reused before attempting splits or workspace creation.
|
||||
- `MAM_WS_LABEL` rename applies exclusively to the chosen target workspace.
|
||||
4. **Version Lockstep v4.4.4**: Maintained consistently across `lib.sh`, `eager.py`, 8 `SKILL.md` manifests, `VERSIONS.md`, and documentation files.
|
||||
5. **Test Suite Verification**: 13/13 targeted tests and 231/231 regression tests passed cleanly with 0 job registry leaks.
|
||||
|
||||
---
|
||||
|
||||
## 2. Review of Commit `0fedae2` (Issue #12: Value-Based Password Scrubbing)
|
||||
|
||||
### 2.1 Value-Based Record Selection
|
||||
- In `.agents/skills/lib_py/delegation/registry.py` (`_cmd_scrub_secrets`):
|
||||
```python
|
||||
# Old:
|
||||
if not (isinstance(broker, dict) and "password" in broker):
|
||||
continue
|
||||
|
||||
# New (0fedae2):
|
||||
if not (isinstance(broker, dict) and broker.get("password")):
|
||||
continue
|
||||
```
|
||||
- **Analysis**:
|
||||
- Legacy records created under previous versions without authentication stored `"password": null`.
|
||||
- Testing `broker.get("password")` ensures only truthy values (non-empty strings containing actual passwords) trigger migration.
|
||||
- Records with `None`, `""`, or omitted `"password"` keys are untouched.
|
||||
- Verified on live workspace: `registry.py --registry-dir .mam/jobs scrub-secrets --dry-run` reports `would scrub 0 record(s) under .mam/jobs`, avoiding unnecessary rewrites.
|
||||
|
||||
### 2.2 Documentation & Specification Alignment
|
||||
- `registry.md`: Clarifies that `scrub-secrets` strips stored passwords, leaving empty/null keys to be dropped naturally on the record's next write via `_atomic_write_record`.
|
||||
- `VERSIONS.md`: Section 2 updated to emphasize that `"password": null` is not treated as a secret and is neither reported nor rewritten.
|
||||
|
||||
### 2.3 Automated Test Coverage
|
||||
- In `tests/test_issue12_registry_secrets.py`, `test_scrub_secrets_cli_migrates_a_registry` was expanded to create an unauthenticated job record with `"password": None`.
|
||||
- Verifies:
|
||||
- `empty not in dry.stdout` (dry run ignores null password).
|
||||
- `empty_path.read_text() == empty_before` (file is not modified).
|
||||
- Only genuine leaked passwords are reported and scrubbed.
|
||||
|
||||
---
|
||||
|
||||
## 3. Review of Commit `ea911d7` (Issue #8: `update.sh` Merge Hardening)
|
||||
|
||||
### 3.1 Install State Trailing Newline Guard
|
||||
- In `deploy/update.sh`:
|
||||
```bash
|
||||
if [ -s .mam/install_state ] && [ -n "$(tail -c1 .mam/install_state)" ]; then
|
||||
printf '\n' >> .mam/install_state
|
||||
fi
|
||||
```
|
||||
Guarantees that an existing `install_state` missing a final newline does not concatenate appended keys onto the last line.
|
||||
|
||||
### 3.2 Non-KV Line Rejection
|
||||
- Lines lacking `=` are skipped safely:
|
||||
```bash
|
||||
case "$line" in
|
||||
*=*) ;;
|
||||
*) continue ;;
|
||||
esac
|
||||
```
|
||||
|
||||
### 3.3 Exact Key Matching without Regex Side-Effects
|
||||
- Previously used `grep -q "^${key}="`, which could interpret regex metacharacters in key names.
|
||||
- Now uses `awk`:
|
||||
```bash
|
||||
if ! awk -F= -v k="$key" '$1 == k { found = 1 } END { exit !found }' .mam/install_state; then
|
||||
printf '%s\n' "$line" >> .mam/install_state
|
||||
fi
|
||||
```
|
||||
- Exact literal string equality `$1 == k` prevents false matches and escapes.
|
||||
- Full cycle verified via `tests/test_issue8_update_state.py`.
|
||||
|
||||
---
|
||||
|
||||
## 4. Review of Commit `ea911d7` (Issue #7: Workspace Packing Edge Cases)
|
||||
|
||||
Two new regression tests were added to `tests/test_issue7_workspace_packing.py`:
|
||||
|
||||
1. `test_idle_pane_in_a_later_workspace_is_reused`:
|
||||
- Configures `w1` as full (4 agents) and `w2` with 1 active agent and 1 idle pane (`w2:p_idle`).
|
||||
- Starts 7th agent: verifies the candidate traversal checks `w1`, continues past overflow, checks `w2`, finds the idle pane, and starts the agent directly on `w2:p_idle` without splitting or creating a workspace.
|
||||
2. `test_ws_label_rename_targets_only_the_chosen_workspace`:
|
||||
- Configures `w1` (4 agents, full) and `w2` (1 agent).
|
||||
- Starts 8th agent with `MAM_WS_LABEL="team-b"`.
|
||||
- Confirms that only the target workspace `w2` is renamed: `[["workspace", "rename", "w2", "team-b"]]`. Preceding full candidate workspaces are untouched.
|
||||
|
||||
Both tests pass and accurately pin critical edge-case behaviors.
|
||||
|
||||
---
|
||||
|
||||
## 5. Version Lockstep v4.4.4 Verification
|
||||
|
||||
All components remain strictly locked at `4.4.4`:
|
||||
- `.agents/skills/lib.sh`: `MAM_VERSION="4.4.4"`
|
||||
- `.agents/skills/lib_py/loop/eager.py`: `FRAMEWORK_VERSION = "4.4.4"`
|
||||
- Frontmatter `version: 4.4.4` across all 8 skills (`create`, `stop`, `resume`, `status`, `monitor`, `delegate-job`, `loop`, `orc-onboard`).
|
||||
- `VERSIONS.md`: Current release v4.4.4, 8-row skills matrix, v4.4.4 changelog.
|
||||
- Badges and headers in `README.md`, `README.ko.md`, `BOOTSTRAP.md`, `BOOTSTRAP.ko.md`, and `MESSAGING.md`.
|
||||
- `lib_py.loop.eager.verify_framework_version()`: Verified OK.
|
||||
|
||||
---
|
||||
|
||||
## 6. Test Suite & Registry Integrity Audit
|
||||
|
||||
### 6.1 Targeted Test Execution
|
||||
Command:
|
||||
```bash
|
||||
.venv/bin/python -m pytest tests/test_issue7_workspace_packing.py tests/test_issue8_update_state.py tests/test_issue12_registry_secrets.py tests/test_version_consistency.py
|
||||
```
|
||||
**Result**: 13 passed in 19.47s (100% success).
|
||||
|
||||
### 6.2 Regression Test Execution
|
||||
Command:
|
||||
```bash
|
||||
.venv/bin/python -m pytest tests/test_herdr_shim_contract.py tests/test_layout.py tests/test_deploy_layout.py tests/test_issue5_phase1_delegation.py tests/test_issue5_phase2a_loop.py tests/test_loop_cli.py tests/test_issue5_phase4_runtime.py
|
||||
```
|
||||
**Result**: 231 passed in 128.99s (100% success).
|
||||
|
||||
### 6.3 Sandboxing & Leak Verification
|
||||
- Baseline `.mam/jobs` file count before execution: **2405**
|
||||
- File count after full test execution: **2405**
|
||||
- Leak delta: **0 files created or modified** in live registry.
|
||||
|
||||
---
|
||||
|
||||
## 7. Conclusion
|
||||
|
||||
The updates in `0fedae2` and `ea911d7` cleanly resolve the N1 finding and further harden update and workspace packing operations. All verification criteria are fully satisfied.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in new issue
Block a user