97 lines
6.9 KiB
Markdown
97 lines
6.9 KiB
Markdown
# Cross Code Review — Job d8354ed6
|
|
|
|
**Review target**: Commit `0fe3b99` ("fix(refactor): remove tracked tmp file, add *.tmp to gitignore, fix herdr_session lookup in reconcile.sh MQTT handler")
|
|
**Cumulative scope**: `8dcb2b2..0fe3b99` (commits `51dcf56` → `8dcb2b2` → `ddd43ec` → `0fe3b99`)
|
|
**Prior reviews**: Job `db1eaa7a` (R-1..R-8), Job `00d79aff` (R-9..R-12)
|
|
**Task goals**: A-1 (workspace-scoped session isolation), A-5 (`HERDR_SESSION_NAME` native naming), `.mam.env` template updates
|
|
**Reviewer**: cline | **Date**: 2026-08-05
|
|
|
|
---
|
|
|
|
## 1. Verification of Prior Findings (R-9 .. R-12 from job 00d79aff)
|
|
|
|
| ID | Finding (from 00d79aff) | Status | Evidence |
|
|
|----|------------------------|--------|----------|
|
|
---
|
|
|
|
## 2. New Findings
|
|
|
|
### R-13 (Medium — Stale user-facing documentation in SKILL.md files)
|
|
|
|
The code migration from `HERDR_SERVER_NAME` → `HERDR_SESSION_NAME` is complete in all shell scripts and Python code, but **4 SKILL.md documentation files** still reference the old naming extensively. These are user-facing docs that agents and humans read to understand how to use the skills.
|
|
|
|
**`create/SKILL.md`** (10 references to `HERDR_SERVER_NAME`, 0 to `HERDR_SESSION_NAME`):
|
|
- Line 38: `echo "Herdr server name: ${HERDR_SERVER_NAME:-default}"` — should be `HERDR_SESSION_NAME`
|
|
- Line 67: `using the HERDR_SERVER_NAME environment variable or the --herdr-server <name> flag`
|
|
- Line 74: `export HERDR_SERVER_NAME=multi-agent-canary` — should be `HERDR_SESSION_NAME`
|
|
- Lines 101-102: Safety rules reference `HERDR_SERVER_NAME` for session stop/delete
|
|
- Line 157: `herdr_server: <HERDR_SERVER_NAME>` — YAML field should document `herdr_session`
|
|
- Lines 170-172: `start_command`/`attach_command`/`kill_command` examples use `HERDR_SERVER_NAME=...`
|
|
- Lines 174-176: Comment explains `HERDR_SERVER_NAME` is what the shim reads — now reads `HERDR_SESSION_NAME`
|
|
|
|
**`stop/SKILL.md`** line 19: References `herdr_server` field and `HERDR_SERVER_NAME` env var.
|
|
**`resume/SKILL.md`** line 19: References `HERDR_SERVER_NAME` env var.
|
|
**`status/SKILL.md`** line 19: References `herdr_server` field and `HERDR_SERVER_NAME` env var.
|
|
|
|
**Impact**: Users following these docs will set the wrong env var (`HERDR_SERVER_NAME` instead of `HERDR_SESSION_NAME`). While the code has backward-compat fallback (`HERDR_SERVER_NAME` is still checked as a legacy fallback), users won't get the intended behavior in fresh environments and the docs are misleading. This is a documentation gap, not a code defect — the code works correctly via fallback chains.
|
|
|
|
### R-14 (Low — New `.tmp` file in working tree)
|
|
---
|
|
|
|
## 3. Cumulative Verification (R-1 .. R-8 from job db1eaa7a)
|
|
|
|
All 8 original findings remain fixed in `0fe3b99` (no regressions introduced):
|
|
|
|
| ID | Status | Notes |
|
|
|----|--------|-------|
|
|
| R-1 | ✅ FIXED | `test_workspace_scope.py` passes workspace + scrubs env — 2/2 PASS |
|
|
| R-2 | ✅ FIXED | `conftest.py` scrubs both env vars; test asserts new + legacy fallback — PASS |
|
|
| R-3 | ⚠️ DOCUMENTED | Env-before-workspace order intentional; WARN on missing workspace |
|
|
| R-4 | ✅ FIXED | YAML lookup includes `herdr_workspace` fallback; slug parity verified |
|
|
| R-5 | ✅ FIXED | `delegate-job` echo uses `$HERDR_SESSION_NAME` |
|
|
---
|
|
|
|
## 4. Test & Syntax Validation
|
|
|
|
| Check | Result | Detail |
|
|
|-------|--------|--------|
|
|
| `bash -n` syntax (8 scripts) | ✅ 8/8 PASS | lib.sh, create_session.sh, delegate-job, reconcile.sh, resume_session.sh, update_yaml_resumed.sh, status.sh, stop_session.sh |
|
|
| `test_workspace_scope.py` | ✅ 2/2 PASS | R-1 fix verified |
|
|
| `test_tier1_unit.py` (create/resume/stop subset) | ✅ 18/18 PASS | All unit tests relevant to this review pass |
|
|
| `test_tier1_unit.py` (status integration tests) | ⏭️ SKIPPED | `test_status_*` tests hang — require live herdr server; pre-existing infra issue unrelated to this commit |
|
|
| `test_challenger_m2.py` (non-mock_herdr subset) | ✅ 3/3 PASS | `test_ls_key_error`, `test_variable_splicing_injection_safety`, `test_export_masking_exit_code_preservation` |
|
|
| `test_challenger_m2.py` (mock_herdr subset) | ⏭️ SKIPPED | 4 tests using `mock_herdr` fixture hang in this environment; pre-existing infra issue |
|
|
| `.tmp` files tracked in git | ✅ NONE | `git ls-files \| grep '\.tmp$'` → empty |
|
|
| `.gitignore` covers `*.tmp` | ✅ YES | Line 16: `*.tmp` |
|
|
| All lookup sites consistent | ✅ YES | 6/6 sites use `herdr_session or herdr_server or herdr_workspace or 'default'` |
|
|
| No stale `HERDR_SERVER_NAME` in scripts | ✅ YES | Only backward-compat fallback references remain (intentional) |
|
|
---
|
|
|
|
## 5. Gate Checklist
|
|
|
|
| # | Requirement | Status | Evidence |
|
|
|---|-------------|--------|----------|
|
|
| A-5 | `HERDR_SESSION_NAME` native naming | ✅ PASS | `_real_herdr` reads `HERDR_SESSION_NAME` ✅; all callers export it ✅; all 6 YAML lookup sites consistent ✅; all script comments updated ✅; backward-compat `HERDR_SERVER_NAME` fallback preserved ✅; SKILL.md docs stale (R-13) but code is correct |
|
|
| A-1 | Workspace-scoped session isolation | ✅ PASS | `derive_workspace_slug` + `resolve_herdr_session` with workspace param ✅; slug parity verified ✅; env-before-workspace order documented (R-3) |
|
|
| — | `.mam.env` template updates | ✅ PASS | `.mam.env.example` uses `HERDR_SESSION_NAME` |
|
|
| — | Syntax validity | ✅ PASS | `bash -n` 8/8 |
|
|
| — | Targeted test suites pass | ✅ PASS | 23/23 runnable tests PASS (8 skipped due to pre-existing infra) |
|
|
| — | Backward compatibility | ✅ PASS | `create_session.sh` writes both `herdr_session` + `herdr_server`; all readers use `or`-chain; `--herdr-server` flag still accepted as alias |
|
|
| — | No new files polluted into repo | ✅ PASS | No `.tmp` files tracked; `*.tmp` gitignored |
|
|
|
|
---
|
|
|
|
## 6. Verdict
|
|
|
|
Commit `0fe3b99` successfully resolves all 4 findings (R-9..R-12) from the prior review:
|
|
- The accidentally committed `.tmp` file is removed and `*.tmp` is now gitignored (R-9 ✅)
|
|
- The missed `reconcile.sh:133` migration spot is fixed — all 6 YAML lookup sites are now consistent (R-10 ✅)
|
|
- Both stale comments in scripts are updated (R-11, R-12 ✅)
|
|
|
|
Combined with the prior commit `ddd43ec` (which fixed R-1..R-8), the full cumulative change `8dcb2b2..0fe3b99` now correctly implements A-1 (workspace-scoped session isolation) and A-5 (`HERDR_SESSION_NAME` native naming) with proper backward compatibility. All shell/Python code is consistent, syntax checks pass, and all runnable tests pass.
|
|
|
|
The only remaining issue is **R-13 (Medium)**: 4 SKILL.md documentation files still reference the old `HERDR_SERVER_NAME` naming (10 references in `create/SKILL.md` alone, 0 references to `HERDR_SESSION_NAME`). While the code works correctly via backward-compat fallback chains, users following the documentation will set the wrong env var. This is a documentation gap, not a code defect, and does not block merge — but should be addressed in a follow-up.
|
|
|
|
**No merge-blocking issues remain.** The code is correct, tested, and backward-compatible.
|
|
|
|
[VERDICT: PASS] |