Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee4c5f5242
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with self._gate.cond: | ||
| base = self._check(lease, action, target, phase="commit") | ||
| self._gate.enter(lease.id) |
There was a problem hiding this comment.
When another thread calls FaultBus.report() for a major or critical fault, it does not acquire _gate.cond, so the report can land after _check() observes no fault but before enter() registers the effect. In that interleaving the gating fault exists before the commit point, yet the effect still starts, unlike the ordering guaranteed for e-stop and revocation. Wire fault reporting into the same gate or otherwise make the fault check-and-registration atomic relative to reports.
Useful? React with 👍 / 👎.
| def _busy(self, lease_id: str | None) -> bool: | ||
| me = threading.get_ident() | ||
| return any( | ||
| tid != me and (lease_id is None or lid == lease_id) for lid, tid in self._in_flight | ||
| ) |
There was a problem hiding this comment.
If two concurrent effects both call estop.engage(), each drain ignores only its own thread's entry and waits for the other effect to leave; neither effect can leave because both are blocked in drain(). The same mutual wait can occur when concurrent effects revoke each other's leases, so an emergency halt can deadlock indefinitely instead of returning after effects drain.
Useful? React with 👍 / 👎.
| try: | ||
| self.ledger.append("permits", base) | ||
| return effect() |
There was a problem hiding this comment.
Concurrent execute() calls append to the shared Ledger outside the gate, but Ledger.append() is not thread-safe. In particular, simultaneous first appends to permits can each emit a series descriptor and overwrite _series["permits"], causing the closing index to undercount records; this is reproducible with a path-backed ledger and concurrent executions. Since the new gate explicitly allows effects to run concurrently, its permit logging must be serialized or the ledger must be made thread-safe.
Useful? React with 👍 / 👎.
| reason = None | ||
| if self.estop.is_engaged: | ||
| reason = "estop" |
There was a problem hiding this comment.
If an e-stop is engaged and then deliberately released before the permit() block exits, this final-state check sees it as clear and emits no violation. That contradicts the new contract that an engagement while the block runs is never silent, and loses the audit record for effects performed during that interval; tracking an engagement generation/event rather than only is_engaged would preserve it.
Useful? React with 👍 / 👎.
| return effect() | ||
| finally: | ||
| self._gate.leave(lease.id) |
There was a problem hiding this comment.
When effect is an async or generator function, this call only creates a coroutine or generator and the finally block immediately removes it from the in-flight gate; the body can then be awaited or iterated after an e-stop or revocation has returned. The generic Callable[[], T] signature accepts these callables without warning, so agent code can bypass the new commit-point guarantee; reject deferred results or provide an API that keeps them registered through completion.
Useful? React with 👍 / 👎.
| t = _later(0.1, lambda: ilk.estop.engage("gauntlet")) | ||
| time.sleep(0.3) # slow rail: authority was fine at the start | ||
| with pytest.raises(EStopEngaged): | ||
| ilk.execute(lease, "pay", "merchant", lambda: calls.append("paid")) |
There was a problem hiding this comment.
These tests assume the timer callback has run after a fixed 300 ms sleep, but threading.Timer provides no scheduling deadline; on a sufficiently loaded CI worker the main thread can wake and call execute() before the timer thread engages the stop, making the success path fail pytest.raises. The revocation test has the same race, so synchronize on an event indicating that the halt completed rather than relying on elapsed wall time.
Useful? React with 👍 / 👎.
| if reason: | ||
| self.ledger.append("violations", {**base, "reason": reason}) | ||
|
|
||
| def execute(self, lease: Lease, action: str, target: str, effect: Callable[[], T]) -> T: |
There was a problem hiding this comment.
The new safe API is absent from the README and quickstart, which still instruct users to perform effects inside permit() even though that path retains the revoke-during-spend race this change is intended to fix. Consequently, integrations following the project's supported examples receive no protection from this patch; update the public examples and guidance to use execute() for irreversible effects, as also required for behavioral changes by CONTRIBUTING.md.
Useful? React with 👍 / 👎.
| self._gate.enter(lease.id) | ||
| try: |
There was a problem hiding this comment.
The effect is inserted into _in_flight before execution enters the try/finally region. If KeyboardInterrupt or another asynchronous exception arrives while the condition context is exiting or in the gap before the try, leave() is skipped and the stale entry remains forever, causing later e-stop engagement or revocation to block indefinitely. Establish the cleanup guard before registering the effect so every successful enter() is paired with leave().
Useful? React with 👍 / 👎.
…gainst the commit point
…ts for the review fixes
…kstart; changelog
Codex review response — all eight findings workedEvery finding was verified against the code at P1 — Synchronize gating fault reports with commit registration · Patched in P1 — Prevent concurrent in-effect halts from deadlocking · Patched in P1 — Serialize concurrent execute ledger appends · Patched in P2 — Record transient e-stop engagements during permit blocks · Patched in P1 — Keep deferred effects in flight until they actually execute · Patched in P2 — Wait for scheduled halts instead of relying on sleep timing · Fixed in P1 — Route the documented action path through execute · Fixed in P2 — Install in-flight cleanup before registration can escape · Patched in Full suite on the updated branch ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2742a98a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inspect.iscoroutine(result) | ||
| or inspect.isgenerator(result) | ||
| or inspect.isasyncgen(result) |
There was a problem hiding this comment.
When a plain effect returns a custom object implementing __await__ (including framework-specific awaitables), all three predicates are false, so execute() returns it and the finally block unregisters the effect. The caller can then await the object after estop.engage() or revoke() has returned, running its deferred body without authority; use the general awaitable protocol check (such as inspect.isawaitable) rather than recognizing only native coroutine objects.
Useful? React with 👍 / 👎.
| # EXERCISE: every action passes the gate. execute() re-proves the gate at | ||
| # the commit point, atomically with starting the effect — a halt that lands | ||
| # while the action is being prepared refuses it instead of racing it. | ||
| ilk.execute(lease, "write", "brief.md", effect) # raises EStopEngaged, LeaseNotIssued, LeaseRevoked, LeaseExpired, or FaultActive otherwise |
There was a problem hiding this comment.
Running the README example as shown raises NameError because effect is never defined, so the newly documented commit-point path cannot reach the gate. Define a concrete function or use a self-contained lambda as the detailed quickstart does.
Useful? React with 👍 / 👎.
| def _busy(self, lease_id: str | None) -> bool: | ||
| me = threading.get_ident() | ||
| return any( | ||
| tid != me and tid not in self._halting and (lease_id is None or lid == lease_id) |
There was a problem hiding this comment.
When an effect for lease A blocks inside revoke(lease_b), its thread is placed in _halting; a concurrent external revoke(lease_a) therefore ignores the still-running A effect and returns immediately. Once B drains, the A effect resumes and can perform more work after revocation of A has returned; a global e-stop can return prematurely in the same way. Only effects participating in the actual wait cycle should be excluded, while unrelated scoped drains must continue counting the enclosing effect.
Useful? React with 👍 / 👎.
The bug (found by gauntlet:
revoke-during-spend)The agent gets a lease and passes the gate. 100 ms later the operator engages the e-stop, while the payment is still being prepared (a 300 ms slow rail). The payment still goes out. Gauntlet scores this SILENTLY_EXECUTED.
Root cause
Interlock.permit()is a context manager that checks e-stop, lease and faults once, at block entry, then yields. The real effect happens later inside the caller's block, and nothing re-checks authority at that point. Nothing orders anestop.engage()orleases.revoke()against an effect that's about to start, either. This is a check-then-act race: authority is proven at T0 and used at T0 + 300 ms.Fix (fail-closed)
Interlock.execute(lease, action, target, effect): it re-runs the full gate at the commit point and registers the effect as in flight atomically, under one condition lock (InFlightGate). If the e-stop or revocation state isn't clear at that moment, it raises andeffectnever runs.EStop.engage()andLeaseManager.revoke()now take the same lock: they set their flag, then wait for already-started effects to drain. When an e-stop call returns, nothing is mid-effect and nothing new can start. Effects still run concurrently, because only check-and-register is serialised. A halt raised from inside an effect doesn't deadlock on its own thread.permit()keeps its API: it now documents that it checks at entry only. If authority is lost inside the block, it appends aviolationsledger record on exit, so the gap is never silent.permitsanddenialsrecords gain aphasefield (entryorcommit).Tests (written first, failing on main)
tests/test_inflight.pyhas 7 tests: the gauntlet e-stop case, the revoke case, a clear-path control, e-stop waiting for a running effect, a 16-thread race (no effect may start afterengage()returns; run 10× locally, all green), thepermit()violation record, and no deadlock when a halt is raised inside an effect.Before (main)
After
Gauntlet, run locally
ilk.execute()for the POST (local, uncommitted)Read this before merging: this PR fixes the gate, but gauntlet's interlock adapter still does its POST inside a
permit()block. For the CI case to go green, gauntlet needs a follow-up PR that (a) bumpsvendor/interlockand (b) switches the adapter toilk.execute(). Both change harness files and require re-pinning gauntlet's evidence, so I haven't opened it.Gauntlet's separate honesty-gate failure (LICENSE changed after the pinned harness) is unrelated to this fix and needs Marcus's decision.