6.9 KiB
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 beHERDR_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 beHERDR_SESSION_NAME - Lines 101-102: Safety rules reference
HERDR_SERVER_NAMEfor session stop/delete - Line 157:
herdr_server: <HERDR_SERVER_NAME>— YAML field should documentherdr_session - Lines 170-172:
start_command/attach_command/kill_commandexamples useHERDR_SERVER_NAME=... - Lines 174-176: Comment explains
HERDR_SERVER_NAMEis what the shim reads — now readsHERDR_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
.tmpfile is removed and*.tmpis now gitignored (R-9 ✅) - The missed
reconcile.sh:133migration 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]