docs(improvements): document Herdr 0.8.0 SHA-1 sanitize, mock alignment and 256/256 review PASS
This commit is contained in:
@@ -0,0 +1,142 @@
|
||||
# Cross-Code Review Report — Job 14187d43
|
||||
|
||||
- **Reviewer**: cline (session: `herdr:canary-projects-multi-agent-mux-creator-cline`)
|
||||
- **Commit under review**: `14b9de1` — `fix(herdr): ensure unique agent name via sha1 truncation and align mock errors`
|
||||
- **Cumulative diff vs base**: clean working tree (`(no changes since base commit)` per brief)
|
||||
- **Scope**: SHA-1 hash truncation for Herdr 0.8.0 name uniqueness (`.agents/skills/lib_py/agents/sanitize.py`, `.agents/skills/lib.sh`), Mock Herdr error output alignment (`tests/conftest.py`), early-abort regex updates (`lib.sh`), and test updates (`tests/test_sanitize_and_mock_errors.py`, `tests/test_sanity.py`).
|
||||
|
||||
---
|
||||
|
||||
## 1. Methodology
|
||||
|
||||
Cross-code review performed across three axes, with Bash↔Python parity as a first-class concern because the sanitize contract is implemented twice:
|
||||
|
||||
1. **Lint / static correctness** — syntax, shell quoting, regex anchoring, duplicate-definition consistency, import resolution.
|
||||
2. **Functionality** — collision-freedom, byte-for-byte Bash↔Python parity, error-class coverage vs. the early-abort regex, lookup-path correctness.
|
||||
3. **Loss / regression** — whether removed assertions reduced coverage, whether the new truncation breaks existing contracts, orphaned code.
|
||||
|
||||
Verification combined (a) direct file reads of all 5 changed files, (b) manual parity/collision computation in both Bash and Python, and (c) execution of the test suite (see §5).
|
||||
|
||||
---
|
||||
|
||||
## 2. Per-File Findings
|
||||
|
||||
### 2.1 `.agents/skills/lib_py/agents/sanitize.py`
|
||||
|
||||
- The legacy `s[:16]-s[-15:]` truncation is replaced with `f"{s[:23]}-{h}"` where `h = sha1(s)[:8]` (23 + 1 + 8 = 32). Hashing the **full pre-truncation string** (not the prefix) is the correct choice: it guarantees that two names sharing both prefix and suffix — exactly the sibling-workspace collision case — still differ.
|
||||
- Empty-string handling returns `"agent"` (line 13), which now matches the Bash copy (line 32). This closes the prior Bash↔Python divergence documented in job `c30845cb` (Bash `"x-"` vs Python `"agent"`).
|
||||
- Minor: the module-level docstring was dropped (replaced by a bare `import hashlib`). A docstring is not required, but its removal is a (cosmetic) loss of inline documentation. **Not blocking.**
|
||||
|
||||
### 2.2 `.agents/skills/lib.sh` (two copies: lines 29–52 and 216–239)
|
||||
|
||||
- Both copies of `_sanitize_herdr_agent_name` were updated **identically** (verified by reading both ranges). Consistency between the library section and the shim section is preserved.
|
||||
- The hash is computed with a tool cascade `shasum → sha1sum → openssl → python3`, and `printf '%s' "$s"` (no trailing newline) is used as the hash input — matching Python's `s.encode('utf-8')`. Parity verified empirically (§5.2).
|
||||
- `agent get` lookup (lines 286–307): the legacy prefix/suffix heuristic `an.startswith(tn[:14]) and an.endswith(tn[-12:])` is correctly replaced with `an == tn or an == stn` where `stn = sanitize_herdr_agent_name(tn)`. The inline Python imports `sanitize_herdr_agent_name` from `lib_py.agents.sanitize`, which resolves because lib.sh exports `PYTHONPATH="$SKILL_DIR"` (line 25). **Correct.**
|
||||
- Early-abort regex (line 528): `"^usage:|unknown option|unknown flag|missing required|invalid_agent_name|^error:"`. The two new alternatives (`missing required`, `invalid_agent_name`) align with the mock's new error strings (`missing required --pane`, JSON `invalid_agent_name`). Anchoring semantics are correct under `grep -E`: `^usage:` and `^error:` bind only to their alternatives; `invalid_agent_name` is an unanchored substring match that catches the JSON error payload. **Correct.**
|
||||
|
||||
- **Observation (non-blocking):** the Bash fallback (`else h=$(python3 ... || echo "00000000")`) would, in the degenerate case where *all* of `shasum`/`sha1sum`/`openssl`/`python3` are unavailable, emit a constant `00000000` suffix for every long name — reintroducing the very collisions this commit fixes. In practice this path is unreachable (`python3` is a hard dependency of lib.sh itself, and `shasum` is always present on macOS / `sha1sum` on Linux), so it is a theoretical robustness note only. A future improvement could hash a disambiguating fallback (e.g. a counter or `${#s}`), but it is **not** a defect for this review.
|
||||
|
||||
### 2.3 `tests/conftest.py` (mock Herdr `agent start` handler)
|
||||
|
||||
- Unknown-flag rejection (lines 425–427): `sys.stderr.write("unknown option: " + ... + "\n"); sys.exit(1)`. Matches real Herdr 0.8.0 `unknown option: --env` format.
|
||||
- Required-arg validation (lines 429–432): `missing required --pane` / `missing required --kind`. Matches the abort-regex alternative `missing required`.
|
||||
- Name validation (lines 434–446): emits a JSON error payload with `error.code == "invalid_agent_name"`, which the abort-regex catches via the `invalid_agent_name` substring. **Aligned with real 0.8.0 output and with the lib.sh abort gate.**
|
||||
- Validation ordering (unknown flags → required args → name) is sound: each early test in `test_mock_herdr_error_formatting_and_abort` hits the intended branch.
|
||||
|
||||
### 2.4 `tests/test_sanitize_and_mock_errors.py` (new)
|
||||
|
||||
- `test_sanitize_sibling_workspace_non_collision`: asserts the 4 real sibling workspaces (all `canary-projects-*-creator-claude`) sanitize to 4 distinct 32-char names. This is a **direct regression test for the collision bug** the legacy `s[:16]-s[-15:]` rule caused. Verified manually (§5.2): the 4 outputs are distinct with differing SHA-1 suffixes.
|
||||
- `test_sanitize_bash_python_parity`: 11 edge cases (incl. empty, digit-leading, underscore-leading, 32- and 33-char boundaries) assert byte-for-byte Bash↔Python equality. **Strong contract test.** All pass.
|
||||
- `test_mock_herdr_error_formatting_and_abort`: exercises all three new error classes and asserts both the exact stderr substring and that it matches the abort regex. **Correct and complete.**
|
||||
|
||||
### 2.5 `tests/test_sanity.py`
|
||||
|
||||
- The old `assert session_name.endswith("-creator-claude")` (herdr-registered name) was removed and replaced with:
|
||||
- `assert len(session_name) <= 32`
|
||||
- `assert session_name == sanitize_herdr_agent_name(sessions[0]["name"])`
|
||||
- **Not a coverage loss.** The old `endswith("-creator-claude")` on the *herdr-registered* name is incompatible with the new (correct) truncation — a long workspace name truncates to `canary-projects-multi-a-039bb460`, which legitimately no longer ends with `-creator-claude`. The replacement assertion is *stronger*: it verifies the herdr agent key equals the sanitized form of the yaml session name, i.e. it pins the sanitize contract across the two stores. The unsanitized yaml name is still checked for `endswith("-creator-claude")` on line 72, so the semantic suffix check is retained where it is actually valid.
|
||||
|
||||
---
|
||||
|
||||
## 3. Cross-Cutting Consistency Checks
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| Bash `_sanitize_herdr_agent_name` == Python `sanitize_herdr_agent_name` (11 cases incl. empty, digit/underscore leading, 32/33-char boundaries) | ✅ Identical (verified by `test_sanitize_bash_python_parity` + manual run) |
|
||||
| Empty-string divergence (job `c30845cb`) resolved | ✅ Both return `"agent"` |
|
||||
| Sibling-workspace collision (the root cause) eliminated | ✅ 4/4 unique 32-char names |
|
||||
| Mock error strings ⊆ lib.sh abort regex | ✅ `unknown option`, `missing required`, `invalid_agent_name` all match |
|
||||
| `agent get` lookup uses sanitized name (no legacy heuristic) | ✅ `an == tn or an == stn` |
|
||||
| Two Bash copies of the function are identical | ✅ Lines 29–52 == 216–239 |
|
||||
| Removed `endswith` assertion compensated by stronger contract assertion | ✅ |
|
||||
| PYTHONPATH for inline `from lib_py.agents.sanitize import ...` | ✅ Set at lib.sh:25 |
|
||||
|
||||
---
|
||||
|
||||
## 4. Issues Identified
|
||||
|
||||
**Blocking issues:** none.
|
||||
|
||||
**Non-blocking observations:**
|
||||
1. **(Robustness, theoretical)** The Bash hash fallback `echo "00000000"` would collapse all long names to the same suffix if every hash tool were unavailable. Unreachable in any supported environment (macOS has `shasum`; Linux has `sha1sum`; `python3` is itself a lib.sh dependency), so not a defect — but a future hardening could disambiguate the fallback.
|
||||
2. **(Maintainability, pre-existing)** `_sanitize_herdr_agent_name` is duplicated in lib.sh (library section + shim section). Both copies are consistent after this commit, so no action is required here, but the duplication remains a drift risk.
|
||||
3. **(Cosmetic)** `sanitize.py` lost its module docstring; behavior is unaffected.
|
||||
|
||||
None of these rise to the level of requiring a fix, and none warrant replanning.
|
||||
|
||||
---
|
||||
|
||||
## 5. Test Verification
|
||||
|
||||
### 5.1 Directly-affected code paths (regression baseline + new tests)
|
||||
```
|
||||
tests/test_herdr_shim_contract.py
|
||||
tests/test_tier1_unit.py
|
||||
tests/test_sanity.py
|
||||
tests/test_sanitize_and_mock_errors.py
|
||||
=> 39 passed in 19.54s
|
||||
```
|
||||
This covers the documented regression baseline (36, per prior jobs `cdd44bb3`/`1c80f10e`) plus the 3 new tests introduced by this commit.
|
||||
|
||||
### 5.2 Manual parity / collision verification (Bash + Python)
|
||||
```
|
||||
canary-projects-educative-export-tools-creator-claude -> canary-projects-educati-9da57e6e (32)
|
||||
canary-projects-getting-started-a2a-creator-claude -> canary-projects-getting-48a65fbb (32)
|
||||
canary-projects-multi-agent-mux-creator-claude -> canary-projects-multi-a-039bb460 (32)
|
||||
canary-projects-pu-riverpod-cookbook-creator-claude -> canary-projects-pu-rive-5e30d500 (32)
|
||||
unique: True (collision eliminated)
|
||||
"" -> "agent" (both Bash and Python; divergence resolved)
|
||||
"exact-32-chars-long-name-1234567" -> unchanged (32, not truncated; both)
|
||||
"123_starts_digit" -> "x-123_starts_digit" (both)
|
||||
```
|
||||
Bash and Python outputs are byte-for-byte identical across all cases.
|
||||
|
||||
### 5.3 Remaining (non-e2e) test files
|
||||
```
|
||||
tests/test_deploy_freshness.py test_deploy_layout.py test_deploy_registry_merge.py
|
||||
tests/test_a4_adapter_contract.py test_b4_session_created.py test_b7_diff_untracked.py
|
||||
tests/test_uuid_target.py test_workspace_scope.py test_o1_rebuttal.py
|
||||
tests/test_o3_scoped_guard.py test_orc_onboard.py test_b8_send_keys_verification.py
|
||||
tests/test_o2_race_free_lock.py
|
||||
=> 181 passed in 172.03s
|
||||
```
|
||||
|
||||
### 5.4 e2e / integration suites (tier2/3/4)
|
||||
```
|
||||
tests/test_tier2_component.py test_tier3_integration.py test_tier4_e2e.py
|
||||
=> 36 passed in 192.70s (0:03:12)
|
||||
```
|
||||
These suites spawn `reconcile.sh --subscribe --idle-timeout 0` loops and are inherently long-running (~3 min). **All 36 pass with 0 failures / 0 errors.** These suites exercise the monitor/reconcile/e2e subsystems, none of which are touched by commit `14b9de1`; their clean pass confirms no collateral regression.
|
||||
|
||||
### 5.5 Aggregate
|
||||
- **Full suite: 256 tests pass** (39 + 181 + 36), with **0 failures, 0 errors** — matching the commit message's "256/256 passed" claim.
|
||||
- Coverage spans every module, including all changed code paths (sanitize truncation, mock errors, abort regex, `agent get` lookup) and all unaffected subsystems (deploy, adapters, reconcile, e2e).
|
||||
- All assertions in the new `test_sanitize_and_mock_errors.py` pass.
|
||||
|
||||
---
|
||||
|
||||
## 6. Verdict
|
||||
|
||||
The commit correctly replaces the collision-prone legacy truncation with a collision-free SHA-1 suffix scheme, keeps the Bash and Python implementations byte-for-byte identical (including the previously-divergent empty-string case), aligns the mock Herdr error output with real Herdr 0.8.0 and with the lib.sh early-abort regex, and simplifies the `agent get` lookup to use the sanitized name. Test changes replace a now-invalid `endswith` assertion with a stronger sanitize-contract assertion rather than weakening coverage. No blocking issues were found; the only notes are theoretical/non-blocking.
|
||||
|
||||
[VERDICT: PASS]
|
||||
+10
-2
@@ -1,9 +1,9 @@
|
||||
# 🛠️ Multi-Agent Mux 종합 개선 및 미해결 과제 백로그 (`IMPROVEMENTS.md`)
|
||||
|
||||
- **최종 갱신일**: 2026-08-14 (Herdr 0.8.0 호환성 패치 반영: Pane 선제할당, agent prompt fast-path, 32자 이름 매핑, status/reconcile 동기화)
|
||||
- **최종 갱신일**: 2026-08-15 (Herdr 0.8.0 세션명 32자 SHA-1 해시 절단 기반 유일성 보장, Mock 0.8.0 에러 포맷 정렬, Early Abort 가드 갱신 및 전체 256/256 회귀 통과 반영)
|
||||
- **통합 관리 대상**: 기존 `CODEBASE_REVIEW_REPORT.md` + `OPTIMIZATION.md`
|
||||
- **총 추적 미해결 과제**: **10건** (아키텍처 2건, 엣지케이스 5건, 오케스트레이션 0건, 레거시 잔재 3건)
|
||||
- **완료된 과제**: **14건** (A-1, A-3, A-5, B-1, B-3, B-4, B-7, B-8, C-1, C-2, O-1, O-2, O-3, O-4-OrcOnboard, Herdr-0.8.0-Compat)
|
||||
- **완료된 과제**: **15건** (A-1, A-3, A-5, B-1, B-3, B-4, B-7, B-8, C-1, C-2, O-1, O-2, O-3, O-4-OrcOnboard, Herdr-0.8.0-Compat-SanitizeHash)
|
||||
|
||||
---
|
||||
|
||||
@@ -196,6 +196,14 @@
|
||||
- bash 3.2 macOS 규격 빈 배열 확장 안전성(`${ARR[@]+"${ARR[@]}"}`) 및 per-iteration budget reset / total budget cap 결함을 완벽히 보완하고 회귀 테스트 `tests/test_o1_rebuttal.py` (10/10 PASS)로 입증했습니다.
|
||||
- `MULTI_AGENT_RULES.md`, `.ko.md`, `multi-agent-mux-loop/SKILL.md` 문서 연동을 완료했습니다.
|
||||
|
||||
### **Herdr-0.8.0-Compat-SanitizeHash: 세션명 32자 SHA-1 해시 접미사 절단 기반 유일성 보장 및 Mock Herdr 0.8.0 에러 정렬 (Commit `14b9de1`)** — ✅ 완료
|
||||
- `.agents/skills/lib_py/agents/sanitize.py` 및 `.agents/skills/lib.sh` 내 `_sanitize_herdr_agent_name()`의 기존 단순 절단 방식(`s[:16]-s[-15:]`)으로 인해 동일 부모 경로 하위의 형제 워크스페이스들이 동일한 세션명으로 축약되던 결함을 **8자리 SHA-1 해시 접미사(`s[:23]-sha1[:8]`)** 결합 방식으로 100% 해소했습니다.
|
||||
- 빈 문자열(`"agent"`), 숫자/특수문자 시작(`"x-"` 접두사), 32자 경계 및 33자 이상 절단 등 11개 엣지 케이스에서 **Bash 심 ↔ Python 모듈 간 100% 바이트 단위 동등성(Parity)**을 확립했습니다.
|
||||
- `tests/conftest.py` Mock Herdr 에러 출력을 실제 Herdr 0.8.0 바이너리 출력 규격(`unknown option:`, `missing required`, `invalid_agent_name`)과 완전 일치하도록 정렬하고, `lib.sh` Early Abort 가드 정규식(`missing required`, `invalid_agent_name`, `^error:`)을 보강하여 불필요한 3회 재시도 루프 지연을 원천 차단했습니다.
|
||||
- `lib.sh`의 `has-session` 분기 내 잔존하던 구형 휴리스틱을 제거하고, `tests/conftest.py` 내의 7곳 구형 절단 사본을 `_match_agent` 헬퍼 함수로 일원화했습니다.
|
||||
- 전용 단위 테스트 `tests/test_sanitize_and_mock_errors.py` (3/3 PASS) 및 전체 회귀 테스트 스위트 (**256/256 PASS, 0 Failures / 0 Errors**)를 검증 완료했습니다.
|
||||
- 멀티에이전트 자율 오케스트레이션 루프(`run_loop.sh --all-reviewer`)를 통해 Creator(`agy`, Job: `94bceedd`) 및 Reviewer(`cline`, Job: `14187d43`) 교차 검증 만장일치 **`[VERDICT: PASS]`** 를 달성했습니다.
|
||||
|
||||
---
|
||||
|
||||
## 6. 🧭 우선순위 실행 로드맵 (Prioritized Execution Roadmap)
|
||||
|
||||
Reference in New Issue
Block a user