fix(mqtt): resolve B-9 by implementing lazy get_logs_dir() evaluation
- Replace import-time LOGS_DIR cwd binding with dynamic get_logs_dir() function - Implement PEP 562 __getattr__ and __dir__ for transparent LOGS_DIR backward compatibility - Update audit-log callers in mqtt_common.py and registry.py to use dynamic resolution - Add 5 regression guards in tests/test_tier1_unit.py (276/276 PASS) - Update IMPROVEMENTS.md, VERSIONS.md, registry.md, and include plan and peer review reports
This commit is contained in:
@@ -0,0 +1,164 @@
|
||||
# Cross-Code Review Report: B-9 (LOGS_DIR import-time cwd freeze fix)
|
||||
|
||||
- **Job ID**: c35385ad
|
||||
- **Reviewer**: cline
|
||||
- **Date**: 2026-08-17
|
||||
- **Backlog Item**: B-9 (P4-1) — `LOGS_DIR` import-time cwd freeze resolution
|
||||
- **Changed Files**: `mqtt_common.py`, `registry.py`, `registry.md`, `tests/test_tier1_unit.py`, `IMPROVEMENTS.md`, `VERSIONS.md`
|
||||
|
||||
---
|
||||
|
||||
## 1. Objective
|
||||
|
||||
Verify that the B-9 implementation correctly refactors `mqtt_common.py` and `registry.py` to resolve the audit-log root (`LOGS_DIR`) dynamically at call time via `get_logs_dir()`, eliminating the import-time `os.getcwd()` freeze that caused audit-log path drift after `chdir`. Backward compatibility for `mqtt_common.LOGS_DIR` consumers must be preserved, 5 dedicated regression tests must be added, and documentation must be accurate.
|
||||
|
||||
---
|
||||
|
||||
## 2. Implementation Review
|
||||
|
||||
### 2.1 `mqtt_common.py` — Core Fix
|
||||
|
||||
**Before:**
|
||||
```python
|
||||
def _default_logs_dir() -> str: ...
|
||||
LOGS_DIR = _default_logs_dir() # frozen at import time
|
||||
```
|
||||
|
||||
**After:**
|
||||
```python
|
||||
def get_logs_dir() -> str:
|
||||
"""Audit-log root, resolved at call time (B-9). ..."""
|
||||
env = os.environ.get("DELEGATE_JOB_LOGS_DIR")
|
||||
if env and env.strip():
|
||||
return env
|
||||
return os.path.join(os.getcwd(), ".mam", "delegate_job_logs")
|
||||
|
||||
def __getattr__(name: str): # PEP 562 (3.7+)
|
||||
if name == "LOGS_DIR":
|
||||
return get_logs_dir()
|
||||
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
|
||||
|
||||
def __dir__():
|
||||
return sorted(set(globals()) | {"LOGS_DIR"})
|
||||
```
|
||||
|
||||
**Assessment:**
|
||||
- The module-level `LOGS_DIR = _default_logs_dir()` assignment is **removed** — verified by AST guard test and manual grep (0 hits at module scope).
|
||||
- `get_logs_dir()` is now **public** (renamed from `_default_logs_dir`), resolving the path per call.
|
||||
- PEP 562 `__getattr__` provides backward-compatible `mqtt_common.LOGS_DIR` access, resolving dynamically each time.
|
||||
- PEP 562 `__dir__` keeps `LOGS_DIR` discoverable in `dir()` and tab-completion.
|
||||
- `__getattr__` correctly raises `AttributeError` for unknown attributes (prevents infinite recursion in `hasattr`).
|
||||
|
||||
**Internal callers updated (all resolve through `get_logs_dir()` when `logs_dir=None`):**
|
||||
| Function | Line | Pattern |
|
||||
|---|---|---|
|
||||
| `job_log_dir` | 441 | `Path(logs_dir or get_logs_dir()) / job_id` |
|
||||
| `job_log_path` | 444 | delegates to `job_log_dir` |
|
||||
| `append_event` | 488 | delegates to `job_log_path` |
|
||||
| `init_job_log` | 523 | delegates to `job_log_dir` |
|
||||
| `update_logged_status` | 505 | delegates to `job_log_path` |
|
||||
| `read_logged_meta` | 553 | delegates to `job_log_path` |
|
||||
| `read_logged_status` | 561 | delegates to `job_log_path` |
|
||||
| `iter_logged_events` | 572 | delegates to `job_log_path` |
|
||||
| `list_logged_jobs` | 589 | `Path(logs_dir or get_logs_dir())` |
|
||||
|
||||
All 9 audit-log functions chain through `get_logs_dir()` when no explicit `logs_dir` is passed. **No stale `or LOGS_DIR` (bare global) references remain.**
|
||||
|
||||
### 2.2 `registry.py` — Consumer Updates
|
||||
|
||||
Two references updated from `mqtt_common.LOGS_DIR` to `mqtt_common.get_logs_dir()`:
|
||||
- Line 198 (`get_feedback`): `logs_dir = mqtt_common.get_logs_dir()` ✓
|
||||
- Line 389 (`_cmd_logs`): `logs_dir = args.logs_dir or mqtt_common.get_logs_dir()` ✓
|
||||
|
||||
### 2.3 `registry.md` — Documentation
|
||||
|
||||
Helper list updated to describe `get_logs_dir` as the primary API with `LOGS_DIR` noted as a "dynamic compat alias". Accurate and consistent with the implementation.
|
||||
|
||||
### 2.4 Backward Compatibility
|
||||
|
||||
- `from mqtt_common import LOGS_DIR` — **0 occurrences** in the entire repository (verified by grep). Compat surface 100% covered by `__getattr__`.
|
||||
- `mqtt_common.LOGS_DIR` attribute access — preserved via PEP 562 `__getattr__`, resolves dynamically.
|
||||
- `DELEGATE_JOB_LOGS_DIR` env override — now reflected at call time (bonus improvement, not a regression).
|
||||
|
||||
### 2.5 Test Suite (`tests/test_tier1_unit.py`)
|
||||
|
||||
5 new B-9 regression tests (lines 406–473):
|
||||
|
||||
| Test | Guard Type | What It Verifies |
|
||||
|---|---|---|
|
||||
| `test_b9_logs_dir_follows_cwd_changes` | T1 (dynamic) | `get_logs_dir()` and `LOGS_DIR` compat alias follow `chdir` |
|
||||
| `test_b9_audit_log_lands_under_the_current_cwd` | T3 (file creation) | Actual `meta.json` file appears under current cwd (catches swallowed errors) |
|
||||
| `test_b9_logs_dir_env_override_is_dynamic` | Env dynamic | `DELEGATE_JOB_LOGS_DIR` honored at call time; clearing restores cwd default |
|
||||
| `test_b9_no_module_level_logs_dir_binding` | T1 (AST static) | No module-level `LOGS_DIR` assignment (covers `Assign` and `AnnAssign`) |
|
||||
| `test_b9_logs_dir_stays_discoverable` | C2-a (PEP 562) | `LOGS_DIR` in `dir()`, `hasattr` works, no duplicates |
|
||||
|
||||
The T3 guard is particularly well-designed — it asserts the **actual file** appears on disk, not just string equality. This is critical because the audit-log layer uses best-effort `except Exception` that swallows errors silently.
|
||||
|
||||
### 2.6 Documentation (`IMPROVEMENTS.md`, `VERSIONS.md`)
|
||||
|
||||
**IMPROVEMENTS.md:** Header updated (date, 276/276, 24 completed, 1 open). B-9 moved to completed section (lines 84–90). Edge-case section shows "0건 — 전원 완료". ✓
|
||||
|
||||
---
|
||||
|
||||
## 3. Verification Results
|
||||
|
||||
### 3.1 Syntax Checks (`py_compile`)
|
||||
| File | Result |
|
||||
|---|---|
|
||||
| `mqtt_common.py` | ✅ PASS |
|
||||
| `registry.py` | ✅ PASS |
|
||||
| `tests/test_tier1_unit.py` | ✅ PASS |
|
||||
|
||||
### 3.2 Stale Reference Scan
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| `from mqtt_common import LOGS_DIR` in source | ✅ 0 occurrences |
|
||||
| Bare `or LOGS_DIR` (global) in source | ✅ 0 occurrences |
|
||||
| Module-level `LOGS_DIR =` assignment | ✅ 0 occurrences (removed) |
|
||||
|
||||
### 3.3 B-9 Targeted Tests
|
||||
```
|
||||
tests/test_tier1_unit.py::test_b9_logs_dir_follows_cwd_changes PASSED [ 20%]
|
||||
tests/test_tier1_unit.py::test_b9_audit_log_lands_under_the_current_cwd PASSED [ 40%]
|
||||
tests/test_tier1_unit.py::test_b9_logs_dir_env_override_is_dynamic PASSED [ 60%]
|
||||
tests/test_tier1_unit.py::test_b9_no_module_level_logs_dir_binding PASSED [ 80%]
|
||||
tests/test_tier1_unit.py::test_b9_logs_dir_stays_discoverable PASSED [100%]
|
||||
5 passed, 30 deselected in 0.10s
|
||||
```
|
||||
|
||||
### 3.4 Full Test Suite
|
||||
```
|
||||
276 passed in 422.40s (0:07:02)
|
||||
```
|
||||
**Zero failures, zero errors, zero regressions.** Test count increased from 271 → 276 (+5 new B-9 tests), consistent with documentation claims.
|
||||
|
||||
---
|
||||
|
||||
## 4. Lint & Quality Assessment
|
||||
|
||||
- **No unused imports** introduced by the change.
|
||||
- **No dead code** — `__getattr__` and `__dir__` are both exercised by tests.
|
||||
- **PEP 562** is the idiomatic Python ≥3.7 pattern for dynamic module attributes; test environment runs Python 3.9.6.
|
||||
- **Thread safety**: `get_logs_dir()` calls `os.environ.get()` and `os.getcwd()`, both thread-safe in CPython. Per-call overhead is negligible vs. the file I/O it precedes.
|
||||
- **No surgical-change violations**: every changed line traces directly to the B-9 requirement.
|
||||
|
||||
---
|
||||
|
||||
## 5. Concerns & Observations
|
||||
|
||||
1. **Minor (non-blocking):** `__dir__` returns `sorted(set(globals()) | {"LOGS_DIR"})` — `LOGS_DIR` would still appear in `dir()` even if `__getattr__` were removed. Purely cosmetic; the AST guard test catches actual binding regressions.
|
||||
2. **No escalation needed:** The fix is a clean, surgical refactor. No design-level rework required.
|
||||
|
||||
---
|
||||
|
||||
## 6. Verdict
|
||||
|
||||
The B-9 implementation is **correct, complete, and well-tested**:
|
||||
- The import-time cwd freeze is eliminated — `get_logs_dir()` resolves per call.
|
||||
- Backward compatibility is fully preserved via PEP 562 `__getattr__`/`__dir__`.
|
||||
- All internal callers and external consumers (`registry.py`) are updated.
|
||||
- 5 high-quality regression tests guard against regression (AST static guard + file-creation guard).
|
||||
- Documentation (`IMPROVEMENTS.md`, `VERSIONS.md`, `registry.md`) is accurate.
|
||||
- Full test suite: **276/276 PASS**, zero regressions.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in New Issue
Block a user