[codex] Verify auth gate blocked exits - #4232
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements verification for auth-blocked checkpoints in verify_blocked_evidence by retrieving and validating the checkpoint from the store. It also adds comprehensive unit tests and test helpers to support this functionality. The reviewer suggests using an exhaustive match statement over LoopBlockedKind instead of an inequality check to leverage compiler-enforced safety for future enum variants.
| if request.blocked.kind != LoopBlockedKind::Auth { | ||
| // A BeforeBlock checkpoint alone is not sufficient for approval, | ||
| // resource, or dependent-run gates: #3424 requires a durable | ||
| // pending gate/process ref for those block types. Auth gates use | ||
| // the blocked turn state itself as the product-visible pending ref, | ||
| // so verifying the pre-block checkpoint is enough to let the | ||
| // applier persist that state. | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
To ensure safety and maintainability, prefer using an exhaustive match statement over LoopBlockedKind instead of an inequality check. This forces a compile-time error if new variants are added to the enum in the future, preventing silent failures or unhandled cases.
match request.blocked.kind {
LoopBlockedKind::Auth => {}
LoopBlockedKind::Approval
| LoopBlockedKind::Resource
| LoopBlockedKind::AwaitDependentRun => {
// A BeforeBlock checkpoint alone is not sufficient for approval,
// resource, or dependent-run gates: #3424 requires a durable
// pending gate/process ref for those block types. Auth gates use
// the blocked turn state itself as the product-visible pending ref,
// so verifying the pre-block checkpoint is enough to let the
// applier persist that state.
return Ok(false);
}
}References
- Prefer exhaustive enum matching over runtime property-based checks for classifying variants when the set is small and known, as it leverages compiler-enforced safety and prevents loosening type constraints.
e7ba8a0 to
eceb8b1
Compare
eceb8b1 to
0795317
Compare
Review — verify auth gate blocked exitsCombined pass (Claude Code + Codex 5.5 Medium — Auth blocked-evidence isn't bound to the actual gate
Low
What's goodAuth carve-out maps correctly to Coverage gapsAuth with a wrong-kind checkpoint at the same Codex 5.5 independently flagged the same gate-binding gap as P2. |
…heckpoints (#4232) Closes the Medium finding from the combined Claude/Codex review on PR #4232: Auth blocked-evidence was not bound to the gate that triggered it. A rogue driver could reuse a legitimate Approval/Resource BeforeBlock checkpoint from the same run, label the exit Auth, and it would validate as BlockedAuth. Fix: thread gate_ref through the checkpoint pipeline so that BeforeBlock records carry the gate identity. verify_blocked_evidence now requires the checkpoint's gate_ref to match the blocked exit's gate_ref in addition to kind and state_ref. Changes: - PutLoopCheckpointRequest / LoopCheckpointRecord: add gate_ref Option field (serde default = None for backward-compatible deserialization of legacy records) - LoopCheckpointRequest (host API): add gate_ref Option field (serde default) - CheckpointStage: add write_before_block(gate_ref) variant; write() delegates to a shared write_with_gate_ref() helper so existing callers are unchanged - gates.rs: BeforeBlock checkpoint calls use write_before_block so the gate ref is stored alongside the checkpoint - port_adapters.rs: propagate gate_ref from LoopCheckpointRequest to PutLoopCheckpointRequest - loop_exit_applier.rs: - add gate_ref cross-check in verify_blocked_evidence (Medium fix) - add comment on wildcard arm explaining intentional fail-closed for #[non_exhaustive] LoopBlockedKind variants (Low fix) - Tests: update auth evidence tests to supply matching gate_ref; add thread_checkpoint_evidence_rejects_auth_blocked_checkpoint_gate_mismatch
|
Addressed the combined review findings in Fixed
Tests
Validation |
* Verify auth gate blocked exits with durable checkpoints * fix(reborn): address combined review — bind gate_ref to BeforeBlock checkpoints (nearai#4232) Closes the Medium finding from the combined Claude/Codex review on PR nearai#4232: Auth blocked-evidence was not bound to the gate that triggered it. A rogue driver could reuse a legitimate Approval/Resource BeforeBlock checkpoint from the same run, label the exit Auth, and it would validate as BlockedAuth. Fix: thread gate_ref through the checkpoint pipeline so that BeforeBlock records carry the gate identity. verify_blocked_evidence now requires the checkpoint's gate_ref to match the blocked exit's gate_ref in addition to kind and state_ref. Changes: - PutLoopCheckpointRequest / LoopCheckpointRecord: add gate_ref Option field (serde default = None for backward-compatible deserialization of legacy records) - LoopCheckpointRequest (host API): add gate_ref Option field (serde default) - CheckpointStage: add write_before_block(gate_ref) variant; write() delegates to a shared write_with_gate_ref() helper so existing callers are unchanged - gates.rs: BeforeBlock checkpoint calls use write_before_block so the gate ref is stored alongside the checkpoint - port_adapters.rs: propagate gate_ref from LoopCheckpointRequest to PutLoopCheckpointRequest - loop_exit_applier.rs: - add gate_ref cross-check in verify_blocked_evidence (Medium fix) - add comment on wildcard arm explaining intentional fail-closed for #[non_exhaustive] LoopBlockedKind variants (Low fix) - Tests: update auth evidence tests to supply matching gate_ref; add thread_checkpoint_evidence_rejects_auth_blocked_checkpoint_gate_mismatch
Summary
Tests
Base is now reborn-integration; this PR no longer depends on #4231.