fix(codex): resolve peer-review findings R1-R6 and T1-T2

- R1: use herdr send-keys C-c for graceful stop - R2: preserve JSONL fallback discovery when database is empty - R3: apply workspace-scoped SQLite query before bounding results - R4: tighten artifact validation to fail closed on missing/invalid metadata - R5: verify actual rollout presence before reporting resumable in status - R6: remove unconditional drift-B backend command fallback - T1: parameterize artifact metadata regression with verified finder fixture - T2: seed unmanaged and managed sessions in mock herdr for drift-B testing - Add final loop review reports (100% PASS)
2026-09-06 00:18:19 +09:00
1 parent 76f10b2096
commit 47640e2fd7
@@ -0,0 +1,60 @@
+# Job 155f5092 Final Report: Resolution of Follow-Up Findings T1 & T2
+
+- **Job ID**: 155f5092
+- **Reviewer Target**: `planner-reviewer-codex-01` (`ff721ed1`)
+- **Status**: Completed (Both regression test findings T1 and T2 resolved and mutation-verified)
+
+---
+
+## 1. Summary of Changes
+
+### T1 (P2): Invalid-metadata assertions only exercise "file not found"
+- **Problem**: In `test_codex_verify_artifact_metadata_fail_closed`, the fixture file was placed outside the discovery path without the finder override being active for cases 1–5. Consequently, `verify_artifact` returned `False` on file existence before reaching metadata parsing, which allowed the old buggy verifier (from commit `76f10b2`) to pass the test.
+- **Fix**:
+  - In `tests/test_a4_adapter_contract.py`, updated `test_codex_verify_artifact_metadata_fail_closed` to use pytest `monkeypatch`.
+  - Installed `adapter._find_rollout` monkeypatch override *before* running all assertions.
+  - Added an explicit assertion verifying that `adapter._find_rollout(uuid, ctx)` resolves the fixture file before evaluating malformed contents.
+  - Parameterized invalid metadata cases:
+    - `{}` (empty dict - R4 defect)
+    - `{"payload": {"cwd": ws}}` (missing `payload.id`)
+    - `{"payload": {"id": uuid}}` (missing `payload.cwd`)
+    - `{"payload": {"id": "other-id", "cwd": ws}}` (mismatched session ID)
+    - `{"payload": {"id": uuid, "cwd": "/wrong/path"}}` (mismatched workspace cwd)
+    - `"not-valid-json"` (malformed JSON syntax)
+    - `[]` (non-dict JSON)
+    - `{"payload": "not-a-dict"}` (non-dict payload)
+  - Added database/transcript metadata inconsistency test: DB has conflicting cwd, asserting fail-closed behavior even if rollout matches workspace.
+  - Retained a valid control case with matching DB cwd and transcript cwd, asserting `True`.
+- **Mutation Verification**:
+  - Bound `old_verify_artifact` from commit `76f10b2` in memory: test **FAILED** with `AssertionError: Expected False for payload: {} (assert True is False)`.
+  - With current implementation: test **PASSED**.
+
+### T2 (P2): Unmanaged-session test never supplies a session to drift-B
+- **Problem**: `test_reconcile_drift_b_unmanaged_session_ignored` used a minimal custom mock script that did not support the `herdr ls -F "#{session_name}|#{session_created}"` or `herdr list-panes -F` query shapes used by `reconcile.sh`, meaning drift-B was never evaluated for the session.
+- **Fix**:
+  - In `tests/test_tier1_unit.py`, updated `test_reconcile_drift_b_unmanaged_session_ignored` to use the contract-compatible `mock_herdr` fixture.
+  - Seeded `mock_herdr_state.json` with two sessions running `codex` in the workspace:
+    1. Unmanaged candidate: `"personal-session"` (unmanaged name, no `MAM_MANAGED` env marker).
+    2. Managed positive control: `"test-proj-creator-codex"` (standard managed name pattern).
+  - Asserted that both sessions are discoverable via `herdr ls -F "#{session_name}|#{session_created}"` and `herdr list-panes -t <name> -F "#{pane_pid}|#{pane_current_path}|#{pane_current_command}"` before running reconcile.
+  - Ran `reconcile.sh --once --emit-diff`.
+  - Asserted that `"personal-session"` is ignored (not present in `drifts` as class B, not in `agent-sessions.yaml`).
+  - Asserted that `"test-proj-creator-codex"` is registered as drift class B and added to `agent-sessions.yaml`.
+- **Mutation Verification**:
+  - Temporarily restored the old fallback (`if not agent: if pm_check and pm_check.get("cmd") in ...: agent = ...`): test **FAILED** with `AssertionError: assert not any(d.get("name") == "personal-session" for d in drifts)` (because `"personal-session"` was incorrectly registered).
+  - With current implementation (fallback removed): test **PASSED**.
+
+---
+
+## 2. Validation & Verification
+
+1. `git diff --check`: Clean (exit code 0).
+2. T1 Mutation Check:
+   - Against `76f10b2` `verify_artifact`: FAILED (`AssertionError: Expected False for payload: {}`).
+   - Against current `verify_artifact`: PASSED.
+3. T2 Mutation Check:
+   - Against `76f10b2` fallback in `reconcile.sh`: FAILED (`assert not True` on personal-session registration).
+   - Against current `reconcile.sh`: PASSED.
+4. Focused Test Execution:
+   - `pytest tests/test_a4_adapter_contract.py tests/test_tier1_unit.py -q -k "codex or adapter or facts_bridge or delegate_agent or cli_bridge"`: **39 passed, 61 deselected in 46.23s**.
+   - `pytest tests/test_tier1_unit.py::test_reconcile_drift_b_unmanaged_session_ignored tests/test_version_consistency.py tests/test_tier2_component.py::test_comp_codex_create_and_reconcile_and_stop -q`: **4 passed in 40.15s**.
@@ -0,0 +1,48 @@
+# Follow-up review — ca31d443
+
+Reviewed the cumulative corrections on `support-codex`, HEAD `76f10b2`, with particular attention to T1 and T2 from job `ff721ed1`. Both remaining findings are resolved. No new blocking findings were identified in this review. No production files were edited by this reviewer.
+
+## Findings closed
+
+### T1: Invalid-metadata regression now exercises the verifier
+
+The test installs the rollout finder override before every invalid case and explicitly verifies that the fixture resolves and exists. It covers missing identity/workspace metadata, mismatched identity/workspace, malformed JSON and payload types, database/transcript cwd disagreement, and a valid database/transcript control case. Pytest monkeypatch restores the singleton adapter after the test.
+
+Independent mutation verification: loaded `CodexAgentAdapter.verify_artifact` from `76f10b2` and bound it to the registry adapter in memory, then ran the revised test in a temporary directory. The test correctly failed with `Expected False for payload: {}`. The same revised test passes with the current implementation. This directly closes the previous false-positive regression coverage.
+
+### T2: Unmanaged-session regression now reaches drift-B
+
+The revised test uses the existing mock Herdr fixture, seeds both unmanaged and managed agent records, asserts listing and pane metadata visibility, and checks both emitted drifts and resulting YAML. The managed positive control must register while the unmanaged session must remain absent.
+
+Independent mutation verification: instantiated the actual sandbox and mock Herdr fixtures in a temporary directory, restored only that sandbox's reconcile script to `76f10b2`, and ran the revised test. It correctly failed at `tests/test_tier1_unit.py:1236`: `assert not any(d.get("name") == unmanaged_name for d in drifts)`. This confirms failure occurs at the intended unmanaged-session behavior, rather than during setup or visibility checks. The test passes with the current reconciler.
+
+## Prior production findings
+
+The four production correction files have the same Git blob hashes as in the prior review:
+
+- Codex adapter: `1e7a5c74f968f56521108b95a707fbe07768a07e`
+- Reconciler: `da806b7237e21ee55b9e1b5ea7bec1c72d570f79`
+- Status: `d668bbdea0704ea6ddd5c33b3ddba3a8184b0f48`
+- Stop: `d14ffeb7208caa299737cafc268cc338ff4addd6`
+
+Accordingly, the prior acceptance of R1–R6 stands: control-key stop transport, empty/stale DB fallback, workspace-scoped discovery, metadata validation, stale-row status handling, and managed-session identification. T1/T2 now provide effective regression protection for the two previously uncovered cases.
+
+## Validation
+
+Current run:
+
+`./.venv/bin/python -m pytest tests/test_a4_adapter_contract.py tests/test_tier1_unit.py::test_reconcile_drift_b_unmanaged_session_ignored tests/test_version_consistency.py -q`
+
+Result: **36 passed in 10.71 seconds**, exit 0.
+
+- Both independent old-code mutation checks failed at the intended defect assertions, as detailed above.
+- `git diff --check main`: clean.
+- Inspection confirmed the remaining cumulative changes match the previously reviewed fixes, with the two regression-test corrections described above.
+
+Validation scope: the prior follow-up independently passed 43 focused adapter/lifecycle/status/version tests on the same production fixes. The initial review independently passed all 462 baseline tests on `76f10b2`. Those are historical results, not a claim that this review reran the full suite on the current working tree. This round reran the affected adapter tests, corrected reconciler test, and version checks; only the regression tests changed since the prior accepted production fixes.
+
+## Disposition
+
+T1 and T2 are closed; all findings raised by this reviewer are resolved. No planner escalation is needed. This verdict covers the reviewed working-tree changes and does not itself merge or commit them.
+
+[VERDICT: PASS]
@@ -73,17 +73,33 @@ class CodexAgentAdapter(BaseAgentAdapter):
            return False
        if ctx.epoch and os.path.getmtime(path) < ctx.epoch:
            return False
-        found_cwd = db_cwd
-        if not found_cwd:
-            try:
-                with open(path) as f:
-                    first = f.readline()
-                found_cwd = json.loads(first).get('payload', {}).get('cwd')
-            except Exception:
+
+        # Fail closed on invalid metadata or mismatched id / cwd (R4)
+        try:
+            with open(path) as f:
+                first = f.readline().strip()
+            meta = json.loads(first)
+            if not isinstance(meta, dict):
                return False
-        if found_cwd and workspace_key(found_cwd) != workspace_key(ctx.cwd):
+            payload = meta.get('payload')
+            if not isinstance(payload, dict):
+                return False
+            file_id = payload.get('id')
+            file_cwd = payload.get('cwd')
+            if not file_id or file_id != uuid:
+                return False
+            if not file_cwd:
+                return False
+            if db_cwd and workspace_key(db_cwd) != workspace_key(file_cwd):
+                return False
+            target_cwd = ctx.cwd or ctx.workspace
+            if not target_cwd:
+                return False
+            if workspace_key(file_cwd) != workspace_key(target_cwd):
+                return False
+            return True
+        except Exception:
            return False
-        return True

    def purge_artifacts(self, uuid: str, ctx: DiscoveryContext) -> List[str]:
        purged = []
@@ -126,7 +142,13 @@ class CodexAgentAdapter(BaseAgentAdapter):
        return os.path.exists(f"{codex_home}/auth.json") or bool(os.environ.get('OPENAI_API_KEY'))

    def discover(self, ctx: DiscoveryContext) -> List[str]:
+        target_ws = ctx.workspace or ctx.cwd
+        target_key = workspace_key(target_ws) if target_ws else ""
+
        db = self._state_db(ctx)
+        candidates = []
+        seen = set()
+
        if db and os.path.exists(db):
            try:
                conn = sqlite3.connect(db)
@@ -135,24 +157,53 @@ class CodexAgentAdapter(BaseAgentAdapter):
                cols = {r[1] for r in cur.fetchall()}
                order_col = "updated_at" if "updated_at" in cols else ("created_at" if "created_at" in cols else None)
                order_clause = f"ORDER BY {order_col} DESC" if order_col else ""
-                rows = conn.execute(
-                    f"SELECT id, cwd FROM threads {order_clause} LIMIT 50"
-                ).fetchall()
+
+                # Query database with workspace scope before bounding (R3)
+                if "cwd" in cols and target_ws:
+                    direct_cwds = list({target_ws, os.path.realpath(target_ws)})
+                    placeholders = ",".join("?" * len(direct_cwds))
+                    cur.execute(f"SELECT id, cwd FROM threads WHERE cwd IN ({placeholders}) {order_clause} LIMIT 50", direct_cwds)
+                    for uuid, cwd in cur.fetchall():
+                        if uuid and uuid not in seen and self.verify_artifact(uuid, ctx):
+                            candidates.append(uuid)
+                            seen.add(uuid)
+
+                # Page through batches if needed to catch symlink aliases / canonical paths
+                if len(candidates) < 50:
+                    cur.execute(f"SELECT id, cwd FROM threads {order_clause}")
+                    while True:
+                        batch = cur.fetchmany(100)
+                        if not batch:
+                            break
+                        for uuid, cwd in batch:
+                            if uuid in seen:
+                                continue
+                            if target_key and cwd and workspace_key(cwd) != target_key:
+                                continue
+                            if uuid and self.verify_artifact(uuid, ctx):
+                                candidates.append(uuid)
+                                seen.add(uuid)
+                                if len(candidates) >= 50:
+                                    break
+                        if len(candidates) >= 50:
+                            break
                conn.close()
-                candidates = []
-                for uuid, cwd in rows:
-                    if ctx.workspace and cwd and workspace_key(cwd) != workspace_key(ctx.workspace):
-                        continue
-                    if uuid and self.verify_artifact(uuid, ctx):
-                        candidates.append(uuid)
-                return candidates
            except Exception:
                pass
+
+        # R2: If database yielded usable candidates, return them; otherwise fall through to transcript discovery
+        if candidates:
+            return candidates
+
        root = self._codex_home(ctx)
-        candidates = []
-        paths = sorted(glob.glob(f"{root}/sessions/*/*/*/rollout-*.jsonl"), key=os.path.getmtime, reverse=True)[:50]
+        paths = sorted(glob.glob(f"{root}/sessions/*/*/*/rollout-*.jsonl"), key=os.path.getmtime, reverse=True)
        for path in paths:
-            uuid = os.path.basename(path)[:-6][-36:]
-            if uuid and self.verify_artifact(uuid, ctx):
-                candidates.append(uuid)
+            base = os.path.basename(path)
+            if base.endswith(".jsonl") and len(base) >= 42:
+                uuid = base[:-6][-36:]
+                if uuid and uuid not in seen and self.verify_artifact(uuid, ctx):
+                    candidates.append(uuid)
+                    seen.add(uuid)
+                    if len(candidates) >= 50:
+                        break
        return candidates
@@ -547,10 +547,6 @@ if herdr_confirmed:
                except Exception:
                    pass

-        if not agent:
-            if pm_check and pm_check.get('cmd') in ('claude', 'agy', 'hermes', 'grok', 'opencode', 'codex'):
-                agent = pm_check['cmd']
-
        if not agent:
            continue

@@ -139,8 +139,6 @@ def resume_on_disk(s):
                    conn.close()
                    if r and r[0] and os.path.exists(r[0]):
                        return 'yes'
-                    if r:
-                        return 'yes'
                except Exception:
                    pass
            m = glob.glob(f"{codex_home}/sessions/*/*/*/rollout-*-{u}.jsonl")
@@ -340,8 +338,6 @@ def resume_on_disk(s):
                    conn.close()
                    if r and r[0] and os.path.exists(r[0]):
                        return 'yes'
-                    if r:
-                        return 'yes'
                except Exception:
                    pass
            m = glob.glob(f"{codex_home}/sessions/*/*/*/rollout-*-{u}.jsonl")
@@ -198,7 +198,11 @@ graceful_stop() {
  pane_pid=$(herdr list-panes -t "$SESSION_NAME" -F '#{pane_pid}' 2>/dev/null | head -1 || true)
  exitkey="$("$(_delegate_py_bin)" -m lib_py.agents exit-key "$AGENT" 2>/dev/null || echo "/exit")"
  echo "graceful: send-keys '$exitkey' to $SESSION_NAME"
-  send_keys_safe "$SESSION_NAME" "$exitkey" "stop$$" || echo "graceful: safe delivery failed (rc=$?) — falling back to kill chain"
+  if [ "$exitkey" = "Ctrl+C" ] || [ "$exitkey" = "C-c" ]; then
+    herdr send-keys -t "$SESSION_NAME" C-c 2>/dev/null || true
+  else
+    send_keys_safe "$SESSION_NAME" "$exitkey" "stop$$" || echo "graceful: safe delivery failed (rc=$?) — falling back to kill chain"
+  fi
  _wait_session_gone "$SESSION_NAME" 5 || true
  if ! herdr has-session -t "$SESSION_NAME" 2>/dev/null; then
    echo "graceful: exited cleanly"