tmpl/multi-agent-mux

Fork 0

Files

T

Godopu fbf275a8bc fix(b4): restore dynamic POSIX timestamp in herdr ls shim and reconcile.sh (100% PASS)

2026-08-08 23:36:50 +09:00

12 KiB

Raw Blame History

🛡️ Cross-Code Review: Job 82d01b13 — B-4 Fix (`session_created` real POSIX timestamp)

Job ID: 82d01b13
Reviewer: cline
Target: Fix B-4 defect in lib.sh and reconcile.sh — ensure the herdr ls shim handler and reconcile.sh return a real POSIX timestamp fallback instead of hardcoded 999999 / epoch 0 for session_created.
Change scope: 4 modified files + 1 new test file (uncommitted working-tree diff).

0. Verdict Summary

The B-4 fix is sound and complete. The core defect (a 0/999999 sentinel that sits below every transcript mtime and silently switches off the stale-transcript guard in verify_session_uuid) is eliminated at both producers: the lib.sh shim ls) handler (F1) and reconcile.sh's parse/registration path (F2). Lint passes, the new 21-case suite is green, and there is no cross-suite regression. Four non-blocking findings are documented below; none require a design-level rework.

[VERDICT: PASS]

1. Change Inventory (verified against working tree)

File	Change	Status
`.agents/skills/lib.sh`	`ls)` shim handler rewritten: `-F` parsing + pane-root-process `lstart` derivation (single batched `ps`, `TZ=UTC LC_ALL=C`) + `now` fallback; `verify_session_uuid` epoch floor restricted to `discover` mode (F6)	✅ verified
`.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh`	`lib_sh` bound unconditionally (F5, fixes drift-C `NameError`); `MAM_EPOCH_FLOOR=1e9`; malformed `created` → `0`; below-floor registration fallback → `int(time.time())` (F2)	✅ verified
`tests/conftest.py`	mock `herdr ls -F` emits `data.get("created") or now` instead of `999999`; `import time` added (F3)	✅ verified
`tests/test_tier3_integration.py`	`ls -F` output now asserted numeric and `> 1_000_000_000` (F4)	✅ verified
`tests/test_b4_session_created.py`	NEW — 21 cases (B-1..B-14, parametrized)	✅ verified

git status --porcelain confirms: M on the 4 tracked files + ?? on the new test (uncommitted — see R-3).

2. Lint / Static Checks

Check	Command	Result
Shell syntax (lib.sh)	`bash -n .agents/skills/lib.sh`	✅ PASS
Shell syntax (reconcile.sh)	`bash -n .../reconcile.sh`	✅ PASS
Embedded Python (ls handler)	`sed -n '587,649p' lib.sh \| python -m py_compile`	✅ PASS (compiles clean)
Sentinel removal	`grep -n '999999' lib.sh reconcile.sh`	✅ only in comments (lib.sh:642, reconcile.sh:358/401); no production emit
`time` import	reconcile.sh:319 `import os, json, glob, subprocess, time, sqlite3`; conftest:82 `import time`	✅ present in both consumers

shellcheck is not installed locally (matches planner §3.3); CI gate remains the authority for that check.

3. Operability / Logic Review

3.1 F1 — shim `ls)` handler (`lib.sh:545-651`)

-F is parsed into format; #{session_name} (name-only) vs else (name|<epoch>) branch. The name|<number> shape is preserved for the else branch, so any consumer parsing the old name|999999 still sees the same shape with a real number. A search confirms no skill script consumes plain herdr ls (all use ls -F), so the format branch is safe (R-4, benign).
Derivation correctly uses shell_pid (pane ROOT process), not foreground_processes[0]. The comment explains the caffeinate -i -t 300 keep-awake trap: fg[0] respawns every 5 min and would creep forward, making an idle session look "created after" its own transcript. This is the key real-world insight and is correctly applied.
Single batched ps -o pid=,lstart= -p <csv> over all pids (one fork, not N) — appropriate for the 15 s reconcile heartbeat (F7).
TZ=UTC LC_ALL=C pins both ps lstart output and mktime parsing. The comment is honest that this is not about ps/mktime skew (they share one TZ) but removes the once-a-year DST ambiguity in local mktime and locale-dependent lstart field names (F8). Correct.
Fallback is safe-direction: starts.get(pid) or now (lib.sh:649). If pid is None (no pane / process-info failed), starts.get(None)→None→now. If ps finds no match or strptime fails, no starts entry→now. A real process epoch is never 0, so 0 or now→now is impossible in practice. The output never degrades to 0/999999 — the B-4 defect class is closed.

3.2 F2 — reconcile.sh parse + registration (`reconcile.sh:388-406, 526-533`)

ls line parse: '|' not in line skip; int(created.strip()) wrapped in try/except ValueError → 0. A malformed field no longer aborts the whole sweep and flips herdr_confirmed=False for every server (the old bare int() raised out of the enclosing try). Correct hardening.
Below-floor (< MAM_EPOCH_FLOOR=1e9) → 0 in herdr_sessions; then in drift-B registration created_epoch = t.get('created') or 0; if < floor: created_epoch = int(time.time()). So a real epoch (>1e9) flows through unchanged; a sentinel/0 becomes now. The registered herdr_session_epoch is therefore always a real time. Correct.
MAM_EPOCH_FLOOR = 1000000000 (2001-09-09) is below every plausible MAM session and far above the 0/999999 sentinels. Well-chosen constant.

3.3 F5 — `lib_sh` unconditional binding (`reconcile.sh:337-342`)

Previously assigned only inside the except NameError branch, which the write path never enters (it predefines d via atomic_dump_yaml). So drift-C's pin raised NameError: name 'lib_sh' is not defined and aborted the sweep in write mode. Now bound unconditionally before the try: d. Correct fix; verified by B-11.

3.4 F6 — `verify_session_uuid` floor restricted to `discover` (`lib.sh:1142`)

epoch = row.get("herdr_session_epoch", 0) if mode == "discover" else 0. In revalidate, epoch=0 (falsy) → the if epoch and mtime(transcript) < epoch guard is skipped, so a resumed session whose transcript legitimately predates the current process is not discarded. In discover, the floor applies and rejects stale transcripts from a previous incarnation. This matches the challenger's (agy) finding and is the correct semantic split. Verified by B-9/B-12.

3.5 F3/F4 — test mock + tier3 assertion

conftest mock ls -F now emits data.get("created") or now (real seconds). This is essential: a mock still emitting 999999 would mask B-4-class regressions. Correct.
tier3 now asserts each ls -F line is numeric and > 1_000_000_000. The previously-discarded res_ls call finally has teeth. Correct.

4. Test Results

Suite	Command	Result
B-4 (new)	`pytest tests/test_b4_session_created.py -q`	21 passed (5.87s)
Targeted tier3 (new assertions)	`pytest tests/test_tier3_integration.py::test_integration_reconcile_diff_formats`	1 passed (1.45s)
Cross-regression	`pytest tests/test_o3_scoped_guard.py tests/test_sanity.py tests/test_b4_session_created.py`	47 passed (17.32s)

The full tier3 suite is long-running (~minutes, per planner §8); the targeted test containing the new ls -F assertions was run directly and passes. The B-4 suite (21 cases incl. parametrized B-7/B-8/B-14) covers sentinel removal, plausible-range, root-process derivation contract, fallback-to-now, sentinel-not-persisted, field sanitization, stale-transcript rejection in discover, registration fallback, drift-C no-NameError, floor-restricted-to-discover, unparseable-exit-nonzero, and caller-TZ consistency.

5. Findings (all non-blocking)

R-1 — F1 derivation path has no effective automated coverage (coverage gap)

The core F1 logic — deriving session_created from the pane root process lstart via batched ps + mktime — is not meaningfully exercised by the suite. In the test environment the mock's pane process-info returns shell_pid = data.get("pid", 9999) (conftest.py:444-452), a fake pid that ps -p 9999 cannot resolve. Consequently starts is always empty and the shim falls through to the now fallback (starts.get(pid) or now, lib.sh:649). Tests B-1..B-5 assert the output contract (no 999999, plausible epoch > 1e9), which now satisfies — so a mutation breaking the ps/strptime/mktime parsing (e.g. wrong format string, wrong field) would not be caught.

Why non-blocking: the fallback direction is safe. An over-estimate (now) only tightens the stale-transcript guard; the B-4 defect (an under-estimate of 0/999999 that switches the guard off) is fixed regardless of whether the primary derivation works. The fix is sound even with this gap.

Recommendation (follow-up): add a test that injects a real OS pid (e.g. spawn a long-lived sleeper, use os.getpid() of a child) into the mock shell_pid, then assert the derived epoch is within a few seconds of that process's actual lstart — distinguishing "derived from lstart" from "fell back to now". The planner's b4mut2 tree (fg[0] usage) reportedly caught B-3/B-4 in a scratchpad tree; the committed mock does not reproduce that because its pids are not live OS processes.

R-2 — `IMPROVEMENTS.md` B-4 entry stale and misstated (doc)

IMPROVEMENTS.md:26 still lists B-4 as the open defect ("시프트 ls의 created=0 하드코딩으로 재개 가드 무력화") with no resolution marker. It also misstates the original sentinel as created=0; the pre-fix code actually emitted 999999 (print(f"{name}|999999")). The planner's §6.1 explicitly recommended "IMPROVEMENTS.md B-4 서술 정정 및 완료 처리" — not done.

Recommendation: mark B-4 resolved and correct 0 → 999999 in the description (or note both: the shim emitted 999999, reconcile fell back to 0).

R-3 — Changes are uncommitted (checkout state)

The entire fix is in the working tree, uncommitted (git status shows M on all 4 files + ?? on the test; git log -S 'MAM_EPOCH_FLOOR' is empty). This is a process/checkout observation, not a code defect — the review is of the diff itself.

Recommendation: commit with a scoped message (e.g. fix(b4): derive real POSIX session_created in herdr ls shim and reconcile).

R-4 — `herdr ls` without `-F` now branches on format (benign)

The shim's ls) handler now branches: fmt == "#{session_name}" → name-only; else → name|<epoch>. Previously it always printed name|999999 for every herdr ls call. The name|<number> shape is preserved on the else branch, and a search confirms no skill script consumes plain herdr ls (all use ls -F). No consumer is affected.

Recommendation: none (documented for completeness).

6. Completeness / 유실 Check

✅ Both producers of session_created (shim F1 + reconcile F2) addressed.
✅ Both sentinels (999999 in shim, 0 in reconcile fallback) eliminated from production paths.
✅ verify_session_uuid consumer guard (F6) correctly scoped to discover so resume is preserved.
✅ drift-C NameError (F5) fixed — the latent crash the challenger surfaced.
✅ Test mock (F3) aligned with production contract; tier3 assertion (F4) enforced.
✅ No orphaned imports/vars: time is used (int(time.time()), time.mktime/strptime); MAM_EPOCH_FLOOR used at both sites; old inline python3 -c "..." fully replaced.
✅ bash -n + py_compile clean; 999999 survives only in explanatory comments.

No 유실 (loss/orphan) issues found.

7. Conclusion

The B-4 fix correctly turns a missing-data sentinel into a derived real timestamp, with a conservative now fallback that can only over-estimate (tightening, never disabling, the stale-transcript guard). The verify_session_uuid floor is correctly narrowed to discover mode so session resume is not broken. The latent drift-C NameError is fixed. Lint and the 21-case suite pass with no cross-regression. The four findings are non-blocking coverage/doc/process observations; the most actionable (R-1) is a test-strengthening follow-up that does not affect the correctness of the shipped code.