104 lines
8.4 KiB
Markdown
104 lines
8.4 KiB
Markdown
# Issue #5 Phase 2a cross-review (`9389d7e`..`0e74aa1`, incl. Round 2 `cb575fa`)
|
||
|
||
- **Reviewer**: `reviewer-creator-grok-01`
|
||
- **Job**: `3f0784c4`
|
||
- **HEAD**: `0e74aa1` (`docs(issue-5): record the evidence requirement, the audit lock and the ack rule`) on `cb575fa` (`fix(issue-5): address phase 2a round 2 findings (F-C, F-D, R1, R4)`)
|
||
- **Tests this turn**: `pytest tests/test_issue5_phase2a_loop.py tests/test_issue5_phase0_contracts.py tests/test_issue5_phase1_delegation.py tests/test_version_consistency.py` → **197 passed** (13.06s). Phase 2a file alone: **88 collected**. Collect-only **664**. Full 664 suite not re-run. `git diff --check 6ad6f08..HEAD` clean.
|
||
|
||
Round-2 findings were re-read in the live sources, not taken from the creator’s notes. Each of F-C, F-D, R1, R4 has a production path and a regression that fails on the pre-fix body.
|
||
|
||
---
|
||
|
||
## 1. Module split (`P2A-01` placement)
|
||
|
||
Plan said `lib_py/loop/engine.py`. Implementation is **`lib_py.delegation.engine`**. That is the freeze-boundary correction: a frozen loop binds `lib_py.delegation` to a different tree than the parent package, so delegation importing a sibling would mix trees. Phase 1’s AST guard would have failed the planned placement. `loop/__init__.py` states the engine lives with the writers. Acceptable plan deviation, not 유실 (Codex `.agents/reports/implementation_plan.md` and root NATS plan untouched).
|
||
|
||
`loop/` holds watcher / policy / shadow / store / attribution / evidence. `test_g9_loop_package_imports_only_delegation_among_siblings` holds.
|
||
|
||
---
|
||
|
||
## 2. CAS boundary (`P2A-01`)
|
||
|
||
`engine.transition` is the only status writer in CAS mode. `_decide` is pure. Lock covers load + decide + one `_atomic_write_record` + queue; `_flush` is **after** release (`engine.py:219-255`). `transition_seq` is incremented only on commit. `test_g5_no_blocking_io_while_the_registry_lock_is_held` spies subprocess/socket/sleep. `test_g6_every_record_writer_is_the_engine_or_a_declared_exception` allows only `register_job`, `next_seq`, and the two `_*_legacy` bodies.
|
||
|
||
`MAM_ENGINE_CAS=0` restores `_update_job_status_legacy` / `_pick_pending_legacy`. X-01..X-04 parametrize `"0"`/`"1"`.
|
||
|
||
`STATUS_TO_REQUEST` maps a merged `status=` onto the request that owns it, so a raw field cannot skip the table. Unknown requests are refused. `status` riding on `progress`/`merge` is refused (`test_r3_*`). `expected_attempt` with no record field fail-closes (`test_r2_*`).
|
||
|
||
Operator requests `redispatch` / `finalize` are gated on `OPERATOR_SOURCES = ("registry_cli", "operator")`. Redispatch must advance `iteration`. Wrapper `_operator_write` uses `_source="registry_cli"`. `publish_event` uses `_source="publish_event"` and cannot leave a terminal (`test_fa_*`).
|
||
|
||
---
|
||
|
||
## 3. Round 2 findings — independently confirmed
|
||
|
||
### F-C (blocking) — idempotent ACK is not a refusal
|
||
|
||
`TransitionResult.idempotent` is a distinct slot from `conflict`. Same-terminal re-receipt sets `idempotent=True`, `commit=False`, no second write (`engine.py:163-167`, `test_fc_engine_marks_an_idempotent_result`).
|
||
|
||
`_operator_write` (`registry.py:329-366`):
|
||
- committed → rc 0
|
||
- idempotent → rc 0 (the wrapper’s `status --set completed` on an already-completed record under `set -e`)
|
||
- genuine conflict → stderr `refused: …`, rc 4
|
||
|
||
`test_fc_idempotent_finalize_is_success_not_refusal` covers both CAS modes. `test_fc_a_genuine_conflict_is_still_reported` keeps cancelled-after-completed at rc 4.
|
||
|
||
### F-D — vocabulary check is not skipped
|
||
|
||
`_operator_write` checks `VALID_STATUSES` **before** the CAS/legacy branch. Invalid status → rc 1, registry fingerprint unchanged, in both `MAM_ENGINE_CAS=0` and `1` (`test_fd_invalid_status_is_rejected_in_both_modes`). This is a usage error, not a CAS refusal (rc 4).
|
||
|
||
### R1 (blocking) — absence of evidence is not approval
|
||
|
||
`collect_observed_identity` (`policy.py:33-65`):
|
||
- identifiers from the `collector_attribution` envelope beside the payload
|
||
- native **herdr** payload identifiers are **not** collected
|
||
- non-herdr (event/filesystem) payloads may name the job/attempt
|
||
- every occurrence is kept (a later contradiction is not dropped)
|
||
- `report_manifest.attempt_id` is collected separately
|
||
|
||
`decide` requires every `ATTRIBUTION_FIELD` to be present **and** agree, then requires a manifest attempt that matches dispatch (`policy.py:115-137`). `finalized_candidate.json` now carries the envelope. Tests: `test_r1_missing_attribution_evidence_holds` (all four fields), `test_r1_missing_report_manifest_holds`, `test_r1_identifiers_inside_a_native_payload_are_not_evidence`, plus the Round-1 contradiction tests.
|
||
|
||
### R4 — compare-and-write is one critical section
|
||
|
||
`update_logged_status` (`mqtt_common.py:580-617`):
|
||
- per-job lock `.<name>.lock`, **outside** the registry transaction
|
||
- read-compare-write under that lock; skip if `existing_seq >= incoming_seq`
|
||
- `tempfile.mkstemp` per write (no shared `.tmp`)
|
||
|
||
`test_r4_overlapping_mirror_flushes_cannot_lose_the_newer_state` intercepts `Path.read_text` after the older flush has read, starts a seq=2 write, asserts the competitor **blocks**, and that `completed` seq=2 survives. `test_r4_each_mirror_write_uses_its_own_temp_file` asserts `mkstemp` in the function body and no leftover temps.
|
||
|
||
---
|
||
|
||
## 4. Shadow does not write (`P2A-03`)
|
||
|
||
`ShadowRunner` is not given `engine`. AST forbids `engine` / `update_job_status` / `transition(` in `shadow.py`. `test_g4_shadow_run_leaves_the_registry_byte_identical` hashes every file under the registry dir before/after `evaluate_all` of all 12 scenarios. Decisions go to `.mam/shadow/<job>.ndjson`. Attribution lives in `.mam/shadow/attribution/`, not the job record (Phase 1 schema freeze). `candidate_approved` is WOULD-COMPLETE log only. Auto-complete is still Phase 2b.
|
||
|
||
---
|
||
|
||
## 5. Watcher (`P2A-02`)
|
||
|
||
`herdr agent wait`: `--timeout` is milliseconds (`timeout_ms`); `--until` always explicit; exit 0 → stdout snapshot; exit 1 → **stderr** (`timeout` / `agent_not_found` healthy; other → `transport_error`). Timeouts do not become `Observation`. Backoff 1→2→4→8→16s; failure **drops** `cached_snapshot`. Tests never invoke a real herdr (`run=` injectable).
|
||
|
||
Default `until=("idle","done","blocked")` is intentional observation of `blocked` for `hold_blocked`. `interactive_ready` is captured but is **not** a completion gate (`policy.py` uses `agent_status in {idle,done}` only).
|
||
|
||
---
|
||
|
||
## 6. Policy vs fixtures (`P2A-04`)
|
||
|
||
12 scenarios (Phase 0 seven + `race_double_dispatch`, `attempt_reuse`, `partial_report`, `unknown_status`, `restart_midflight`) all match `expected_policy.decision` (`test_g7_*`). Debounce without manifest → `hold`. Missing execution anchor → `hold`. Policy is I/O-free (`test_policy_is_pure_no_io_no_clock`).
|
||
|
||
---
|
||
|
||
## Residuals (non-blocking)
|
||
|
||
1. **`pick_pending` CAS split lock** (`registry.py:128-144`): select under lock, then `transition` re-acquires. Concurrent pickers for the **same** session with two pending jobs can both select job A; the loser returns `None` even if job B is still pending (until the next call). Sequential two-job pick still works because the second call re-scans. Retry-on-failed-claim would close it. Not typical production (one picker per session). Safety (no double-claim) is preserved by `expected_status="pending"`.
|
||
2. **`mqtt_common.py:401` docstring** still says ``lib_py.loop.engine``. The module is `lib_py.delegation.engine`.
|
||
3. **`reject_stale` still reads `pane_process_generation` from the native herdr payload** (`policy.py:88-94`; fixtures `restarted_generation`, `race_double_dispatch`). After R1, that field is not evidence for *approval*, but the stale-rejection path still uses it. If a live collector puts generation only in `collector_attribution`, a stale generation becomes `hold` (missing/mismatch) rather than `reject_stale`. Both refuse completion. Vocabulary only.
|
||
4. **`test_r1_identifiers_inside_a_native_payload_are_not_evidence`** plants only three of the four identifiers in the native payload (omits `pane_process_generation`). A regression that *did* count native ids would still hold on the missing fourth field. The production skip of herdr-payload ids is real; the test is weaker than its comment.
|
||
5. Full **664**-test suite not re-run this turn (197 targeted including Phase 0/1). Brief’s “576” is the Phase 1 baseline; collection has grown with Phase 2a (88) plus Round-2 regressions.
|
||
|
||
No `[ESCALATE: PLANNER]`. Engine placement remains a freeze-rule correction. Round-2 fixes are surgical and tested.
|
||
|
||
---
|
||
|
||
[VERDICT: PASS]
|