fix(kernelforge): run opportunity analysis on resumable Codex without global hooks - #1617
Conversation
…ooks Codex sessions need stop_hooks and pre-tool guards for Rewrite opportunity analysis. Write CODEX_HOME/config.toml command hooks that delegate to codex_hook_runner, enable stop_hooks on the Codex provider, and align tests with the Claude hook contract.
|
What it does: Enables Forge Rewrite opportunity analysis on Codex by writing CODEX_HOME managed PreToolUse/Stop command hooks, materializing guard state before each Codex session, and declaring stop_hooks on the Codex provider. Blocking issues: 1
Checked: codex_hook_runner.py, codex_managed_hooks.py, codex.py, opportunity_agent.py (_AnalysisToolGuard), registry.py, test_codex_managed_hooks.py | Ran: pytest test_codex_managed_hooks.py and related registry/lane tests (passed locally) | SKIPPED: full CI matrix |
Apply staging write checks only to write-like tools, cap read/grep like Claude's _AnalysisToolGuard, and allow investigation reads outside staging.
|
Addressed the blocking review note (codex-read-outside-staging).
Pushed on |
PR #1617 -- please re-scopeWhat it does: flips Codex to Blocking issues: 4
Action: re-scope rather than patch. Codex genuinely has no in-session hooks.
Checked: codex.py, codex_managed_hooks.py, codex_hook_runner.py, registry.py, opportunity_agent.py, orchestrator/agent.py, cli.py, insession_gate.py, analysis.py, fusion/author.py, applyback.py, config.py | Ran: generated command via |
PR #1617 -- feat(kernelforge): materialize Forge hooks as Codex managed command hooksWhat it does: Forge Rewrite opportunity analysis refuses any provider that does not declare Blocking issues: 4
Note on CI: SKIPPED: Step 7 independent reader -- no second reader was available; the findings were re-derived against the head tree instead. Checked: codex.py (_child_environment, _sdk_config, _materialize_managed_hooks), codex_managed_hooks.py, codex_hook_runner.py, registry.py, orchestrator/agent.py:595-660 and :840-860, cli.py:_require_lane_provider_capabilities, opportunity_agent.py:_AnalysisToolGuard, base.py:AgentHooks, the three changed test files | Ran: shlex.quote/shlex.split on the generated command; gh run view on the failing shards and on the same workflow on main | Base: 09d1ff5 | Head: 7c99f62 |
Drop the CODEX_HOME managed-hook bridge. Opportunity analysis now accepts providers with stop_hooks or resumable sessions; hookless resumable backends get the same refused-draft resume loop the implementer outer gate uses.
|
Re-scoped per your review (removed the CODEX_HOME managed-hook bridge).
Pushed on |
Apply ruff format to opportunity analysis provider guard.
PR #1617 -- feat(kernelforge): materialize Forge hooks as Codex managed command hooksWhat it does: opportunity analysis refused any backend that does not run Blocking issues: 4
Checked: opportunity_agent.py ( |
Republish staged drafts before each outer Stop check, honor the analysis deadline across resume turns, surface resume failures, and count publications from the recovery loop.
BaoYunkai
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| _repair_staged_task(staging / "draft", repo) | ||
|
|
||
| backend = _HooklessResumableBackend( | ||
| lambda staging: _invalid_staged_draft(staging), |
| return await super().resume(spec, session_id, feedback, usage=usage) | ||
|
|
||
| backend = _SlowHookless( | ||
| lambda staging: _refused_draft(staging), |
| layout = ControllerLayout(tmp_path / "output") | ||
| monkeypatch.setenv("FORGE_AGENT_API_RETRY_BASE_SEC", "0") | ||
| backend = _HooklessResumableBackend( | ||
| lambda staging: _refused_draft(staging), |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| _repair_staged_task(staging / "draft", repo) | ||
|
|
||
| backend = _HooklessResumableBackend( | ||
| lambda staging: _invalid_staged_draft(staging), |
| return await super().resume(spec, session_id, feedback, usage=usage) | ||
|
|
||
| backend = _SlowHookless( | ||
| lambda staging: _refused_draft(staging), |
| layout = ControllerLayout(tmp_path / "output") | ||
| monkeypatch.setenv("FORGE_AGENT_API_RETRY_BASE_SEC", "0") | ||
| backend = _HooklessResumableBackend( | ||
| lambda staging: _refused_draft(staging), |
|
Addressed the re-review on head
CI: Lint ( Please re-review when convenient. |
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
PR #1617 -- fix(kernelforge): run opportunity analysis on resumable Codex without global hooks
What it does: opportunity analysis refused any backend that does not run AgentRunSpec.hooks, so a Codex-only controller run died in OpportunityAnalysisAgent.__init__ before the first turn. The gate is relaxed to stop_hooks or resumable, _AnalysisToolGuard hooks are attached only for providers that execute them, and for a hookless resumable provider _resume_for_refused_staging_drafts drives the Stop refusal from outside the session: republish (quiescent_sec=0.0) so a corrected draft clears its own rejection.json, then, while _on_stop still blocks, resume the session with the refusal text -- bounded by _MAX_STOP_DENIALS and by what is left of the analysis deadline, with resume failures recorded instead of swallowed.
Blocking issues: none.
All four blocking findings from the previous review are closed at this head, re-verified here rather than taken from the diff:
- Stale refusal loop --
publish_complete_staged_tasks(layout, quiescent_sec=0.0, refused=refused)now runs at the top of every iteration, so a draft corrected during a resumed turn clears its refusal. Re-ran my own repro that never unlinksrejection.json(the agent has no tool that can, per the refusal text):resume calls = 1, status = completed, published = 1, was 3 resumes before. The correct-then-withdraw trap is gone too: same repro endspublished = 1instead of an emptytasks_root. The refusal text at :172-180 was corrected to match what the host actually does. - Unbudgeted resumes -- the loop takes
deadline_monotonic, refuses to start a resume with no budget left, caps each turn viareplace(spec, timeout_sec=max(1, int(remaining))), and re-checks the deadline after each resume, settingend_reason="timeout"->ANALYSIS_STATUS_TIMED_OUT.CodexBackend.resumereaches_execute(timeout=spec.timeout_sec)(codex.py:764), so the shrunken cap is enforced on the real provider. - Title/description -- both now describe the diff that is here (outer resume loop, no
CODEX_HOMEhooks), and the test plan names tests that exist in the head tree. - Silent resume failure --
AgentProviderErroris caught explicitly, logged, and turned intoANALYSIS_STATUS_FAILEDwith the provider message inreason;asyncio.CancelledErroris re-raised; the remaining broad catch logs and records the same way its twin atorchestrator/agent.py:696does.
Checked: _resume_for_refused_staging_drafts and the status block in OpportunityAnalysisAgent.run (:587-640), _AnalysisToolGuard._on_stop denial cap and refusal text, task_publisher.pending_rejections / publish_complete_staged_tasks / _write_rejection / _is_withdrawn, codex.py resume/_execute timeout path, registry.py codex capabilities, the three changed test files | Ran: pytest src/kernelforge/tests/kernel_rewrite_controller/test_opportunity_agent.py (28 passed; the one failure, test_agent_staging_always_gets_a_private_git_baseline, reproduces identically at the merge base on this Windows checkout and is unrelated), pytest src/kernelforge/tests/test_lane_session_guarantees.py -k provider (7 passed), plus three repros driving OpportunityAnalysisAgent.run against a fake hookless resumable backend and a real staging layout (correct-on-first-resume, correct-then-withdraw, deadline exhaustion) | Base: 146c113 | Head: eec7704
#1620 added RUF100 to the ruff selection at 19:06; #1617 added a `# noqa: BLE001` to opportunity_agent at 19:07. Each was green on its own branch -- RUF100 did not exist when #1617's checks ran -- and main has been red since they landed 97 seconds apart. BLE001 does not fire on this handler anyway: ruff exempts one that logs through `log.exception`. Only the directive goes; the author's reason stays as a plain comment. The change is comment-only, verified by comparing the token stream with comments stripped. This does not belong to this PR's subject and is here only because the merge brought main's red lint onto the branch, where it blocks review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two landed 97 seconds apart. #1620 stopped BLE001 from firing on a handler that calls log.exception, and #1617 was written before it, so the directive it carries is unused and RUF100 reds every branch that merges main. Carried here rather than in its own PR to unblock this one; it reverts no behaviour and the comment it removes explains a suppression that no longer suppresses anything. Co-authored-by: Cursor <cursoragent@cursor.com>
#1609 landed the same way #1617 did: written before #1620 stopped BLE001 from firing on a handler that re-raises, merged after it, so the directive is unused and RUF100 reds main and every branch that merges it. Same reason as the previous one for carrying it here rather than in its own PR. Co-authored-by: Cursor <cursoragent@cursor.com>
…it (#1595) * Bound the wait on a Ray round so an unschedulable task cannot park a thread _await_or_cancel polled ray.wait inside a `while True` with no wall-clock deadline, relying entirely on someone else cancelling the scope. When Ray cannot schedule the task at all, nobody ever does. Observed 2026-09-21: Ray reported 8.0/8.0 GPU in use while the physical GPUs sat at 0% with no serving processes -- ghost reservations left by a specialist that died without releasing. Two integrate_patch warmups requesting {'CPU': 1.0, 'GPU': 4.0, 'serving_slot': 1.0} stayed in PENDING_NODE_ASSIGNMENT forever, and py-spy found two worker threads parked in this loop for over an hour. Each held its pool slot, so its task stayed 'running', its lane leases lapsed unreclaimed, and the coordinator went silent for 64 minutes. The deadline is the round's own cap plus slack for what actually happens outside it, configurable via INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC. Bailing out goes through the cooperative cancel sequence the cancel path already uses -- ask the actor first, escalate only after the grace period -- because ray.kill runs neither __ray_terminate__ nor atexit and would strand the served process tree of a round that is genuinely still running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reclaim lane leases from holders that already ended Lane occupancy was decided by `SELECT lane, holder_id FROM leases` with no regard for whether anybody was still using the row. Rows carry an expires_at, but release happened only through an explicit DELETE, so a holder that ended outside its release path held its lanes for the life of the session. Observed 2026-09-21: a specialist raised ExecutionCleanupUnconfirmed and the dispatcher deliberately retained capacity rather than releasing. Its six lanes were still held two hours later with 19 integrate_patch tasks starved behind them; the session made no progress until the rows were deleted by hand, twice. Reclamation keys on the holder's task being terminal, not on the TTL. A TTL is a static per-action budget that nothing enforces -- `explore` budgets 7200s and holds server_lifecycle plus benchmark_lane across a benchmark that can exceed it -- so a lapse says only that a long run outlived its estimate, while the terminal state is the holder's own account of having stopped. Reading no timestamp also keeps clock skew out of the predicate entirely. Two guards come straight from reap_dead_holders: owner_scope must match this boot/PID namespace, and a task with a live gpu_leases row is left alone, because a lane records its coordinator rather than the specialist's GPU worker. The sweep shares the caller's cursor so it and the capacity read that follows sit in one BEGIN IMMEDIATE, and it runs as its own pass too -- the dispatcher's lane gate reads lane_holders before any acquire, so a leaked row starves the queue without an acquire ever running to reclaim it. Running ahead of the round rules, this also settles rounds that a leaked lane row used to wedge open indefinitely, either by hand-off or by expiry on the terminal-holder cap. Both outcomes are pinned, as is the bound: a holder whose cleanup never confirmed gets its lanes back without its round being advanced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Break the import cycle instead of deferring around it CodeQL flagged the deferred `from ..state.task_registry import TERMINAL_STATES` inside resource_lock as the start of an import cycle, and it is right: the cycle is real (task_registry imports resource_lock's SqliteLeaseBackend), the deferral only hid it from the runtime. The states are a vocabulary, not behaviour, so they move to a storage-free `state/task_states` that anything may import. task_registry re-exports them, so every existing import site is untouched, and resource_lock now imports at module level with no cycle to step around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reclaim a lane only once its holder's process group is provably empty The previous commit treated a terminal task as proof that nobody was using its lane. An external review showed that inverts the contract: when cleanup is unconfirmed, sub_agent_runner deliberately SKIPS the release and writes terminal anyway, because the process tree may still be alive and the lane is what stops conflicting work from starting. Reclaiming on terminal alone deletes exactly the ownership that was retained on purpose -- and the gpu_leases exemption does not cover it, since most of the nine ExecutionCleanupUnconfirmed raise sites have no GPU lease at all. Reclamation now needs one of two proofs: cleanup confirmed, or a recorded process group with nothing left under it. Both launch sites spawn with start_new_session, so the id is the group's and stays meaningful after the root exits -- which is why it catches the common survivor, a child left in the dead root's group. The evidence key and the helpers say "process group" rather than "pid" because that is what is actually checked; a targeted build records the pgid its handle already carries. Reclamation never signals a process; it only asks. What it cannot see is documented rather than papered over: a descendant that calls setsid leaves both the group and the tree, and nothing in /proc ties it back. A per-spawn cgroup would name it and is a separate project. A test pins the gap so it reads as known rather than missed. Anything unprovable keeps its lane, with no TTL or age heuristic anywhere in the predicate -- silence is not proof. Since lane leases live in a per-session database, a fresh run can never inherit such a row; only resuming an old session meets one. So the operator story is the diagnostic: one warning per row, de-duplicated, naming the lane, the holder, why it cannot be verified, and a paste-ready sqlite statement against this session's real database path, prefaced by the caution to confirm no process of that task is still running. A count rides along in the maintenance summary. That report deliberately does not reuse the reclamation query. Its INNER JOIN on tasks would drop a holder pruned out from under its lease, and its gpu_leases exemption would silence the GPU path -- the one release_resources fails on, and the shape of the 2026-09-21 incident. Reclamation must respect both filters; reporting must not. The rows leaked on 2026-09-21 are not healed by this: the code that wrote them recorded no group, so they take the unprovable path and keep their lanes. What this ends is the recurrence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop trying to prove a lane is free; hold it and tell the operator Three proofs were tried and each was shown by probe to be a proxy a real process slips out of. Terminal state says the task stopped, not its children. An empty spawn process group says nothing: a served process is setsid'd by design, as _server_lifecycle notes where it reads a pidfile, so it leaves that group. A pidfile naming no live server says nothing either -- it is written only after the server answers, leaving the whole model-load window unnamed, teardown unlinks it unconditionally, and matching a cmdline is the same kind of guess one level down. The asymmetry decides it. A lane held too long stalls a queue until an operator spends ninety seconds; a lane released too early puts two rounds on the same cards, which corrupts quietly and may never be noticed. Closing the gap for real needs an identity a descendant cannot escape -- a per-execution cgroup -- and that is its own project. So reclamation by inspection is gone. What remains is liveness, which settles itself (reap_dead_holders is untouched), and a diagnostic for everything else: one warning per (lane, holder), naming the lane, the holder, why it cannot be verified, the spawn process group as a lead explicitly not offered as proof, and a paste-ready sqlite statement against this session's real database path, prefaced by the caution to confirm no process of that task is still running. A count rides in the maintenance summary each tick. Every shape is reported, including ones earlier revisions kept quiet: a holder still winding down (no longer distinguishable from an abandoned one, and the noise is the price), a holder pruned out from under its lease, a holder still on its GPU cards, a row from another boot, and a holder that confirmed cleanup yet left its row behind -- which should not happen and so is worth hearing. 2026-09-21 would still wedge under this code. It would announce itself in the log within a tick, with the command to clear it, instead of taking an hour of py-spy and sqlite to find. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep a timed-out round's GPUs reserved instead of handing them back The wall-clock ceiling killed the actor once the grace passed, which returns its GPUs to the Ray scheduler. But ray.kill runs neither __ray_terminate__ nor atexit, so a round that really was still running keeps its server subprocesses -- and the next round is then placed on cards a live server still maps. That trades a bounded wait for concurrent GPU use, which corrupts quietly and may never be noticed. The timeout path now abandons the round and leaves the actor alive. Its devices stay reserved, so nothing else can be placed on them, and the caller still gets the subprocess.TimeoutExpired its handlers already expect. The cost is that those GPUs stay out of circulation until the session ends or an operator intervenes; the log says exactly that, and what to check before killing the actor by hand. This is the same asymmetry the lane leases settle on, applied to the same kind of resource: a resource stuck is recoverable in ninety seconds, a resource shared is not recoverable at all. The cancel path keeps the behaviour it had on main -- an operator or a shutdown asked for that teardown, and the caller is waiting on it rather than on a round. Also finishes removing the rejected inspection design: _ended_holder_rows was left behind as unused reclamation-only code, and the comments and tests around the recorded process group still described it as something a later reaper would probe. Nothing probes it. It is recorded, and now documented, as a lead for the operator who has to clear a retained lane by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make the timed-out lease quarantine survive its caller's teardown Keeping the actor alive inside _await_or_cancel was not a quarantine. Every real caller — baseline, explore, integrate_patch — closes the lease in a finally:, and close() kills the actor once its stop request fails, handing those GPUs back to the scheduler while the served tree may still map them. run_grid then moves to the next variant and is placed on them. The state was declared in one method and undone in the next. ServingLease now carries the quarantine explicitly, and all three entry points honour it. close() leaves the actor alive and says why. ensure() raises ServingLeaseQuarantined rather than minting a replacement actor behind its back. run_session_kill refuses the round and reports it the way an ensure failure is reported — deliberately not raising, because run_grid moves to the next variant on a non-zero rc and each of those attempts has to be refused too, whereas raising would escape the variant loop and widen the blast radius past what this change is entitled to. The cancel path is untouched: an operator or a shutdown asked for that teardown and the caller waits on it, rather than on a round nobody answered. Also finishes the documentation cleanup. The evidence-key block still called the recorded group the only durable trace a later reaper has, and warned that key drift would stop lanes being freed; two tests were named and documented the same way. No reaper probes the group and these lanes are never freed automatically. Both are now described as what they are: the lead an operator gets for a lane that is retained on purpose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Hold the quarantined actor at session scope, not on the lease object The quarantine guarded all three entry points but only for as long as the ServingLease object lived. Every real owner keeps that lease in an action-local variable, closes it in a finally: and drops it. A Ray actor lives as long as a handle to it does, so losing the last reference collects the actor and returns its GPUs to the scheduler exactly as ray.kill would have -- the quarantine was undone by ordinary garbage collection rather than by any code path. The handle is now parked in a module-level list when the quarantine is declared, which for these purposes is session scope: the process holding it is the session. Nothing removes entries, deliberately. A test drops the lease and forces a collection, and fails without that parking. Also finishes the documentation pass. Three places still said a recurrence carrying a process-group id could be settled by a probe -- the reconcile docstring, its report attribute, and the maintenance summary -- which the final design contradicts: nothing probes that id, and no coordinator-owned lane is reclaimed automatically. What changed on 2026-09-21's shape is not that it heals, but that it announces itself within a tick with the statement that clears it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Measure how often teardown goes unconfirmed, before anyone builds for it leases_unverifiable says how many lanes are held right now. It cannot say whether that is a rare accident or the ordinary outcome, and that is the question which decides what to do next. Three portable ways to release such a lane automatically were designed and measured against the environments this actually runs in, and all three are undeployable on the production path: a per-task cgroup needs a delegated writable subtree that claw mode does not have; a PID namespace needs a fresh procfs, and mount() returns EPERM there; one Kubernetes pod per specialist does not exist in local mode. The only candidate left -- an inherited descriptor sealed behind an unprivileged seccomp filter, because a bare descriptor can be closed by the workload -- is safety-critical to get right, since a filter that misses one fd-destruction route would release lanes on a guarantee that does not hold. So measure first. The ratio rides the maintenance summary next to the count it explains, reads what _write_terminal already records, costs one query and writes nothing. A task whose history carries no cleanup account at all counts as unconfirmed: that is exactly the shape which strands a lane. If stranding turns out to be rare, the diagnostic is sufficient and nobody should take the risk. If it is routine, this is the number that argues for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address review: document the ceiling knob, fix a garbled docstring, drop a swallow Three findings from review, all verified against the tree before acting. The wall-clock ceiling's override was invisible. INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC appeared only in source, so an operator who needed to widen it -- or to reproduce the stall deliberately -- had nowhere to look. Documented in both places, with the semantics taken from the code rather than the PR text: the unset default is the round's own timeout plus the slack, <= 0 disables the ceiling and is also what an uncapped round gets, and a non-finite value is rejected rather than tolerated, because nan > 0 is false and one typo would otherwise switch off the very ceiling that prevents the stall. _unverifiable_holders' docstring had two sentence halves merged into one line, left by an earlier edit. It is the function the PR puts forward as the operator's only route to a remedy, so a garbled sentence in it is worse there than elsewhere. maintenance.py wrapped the new cleanup_unconfirmed read in a broad except that demoted any failure to debug. AGENTS.md forbids that, and it would have hidden exactly the wrong thing: the summary would still look complete while missing the one ratio the retention decision rests on -- including if _locks were ever renamed, since the call reached into the reconciler's private attribute. The swallow is gone and the call now goes through a Reconciler method, so the summary loses its tick loudly instead. The maintenance test's reconciler double gains the method, which is what surfaced the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop a noqa directive that became unused between two merges #1620 added RUF100 to the ruff selection at 19:06; #1617 added a `# noqa: BLE001` to opportunity_agent at 19:07. Each was green on its own branch -- RUF100 did not exist when #1617's checks ran -- and main has been red since they landed 97 seconds apart. BLE001 does not fire on this handler anyway: ruff exempts one that logs through `log.exception`. Only the directive goes; the author's reason stays as a plain comment. The change is comment-only, verified by comparing the token stream with comments stripped. This does not belong to this PR's subject and is here only because the merge brought main's red lint onto the branch, where it blocks review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Revert "Drop a noqa directive that became unused between two merges" This reverts commit 77cb461. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 participants
Problem
Forge Rewrite opportunity analysis (
OpportunityAnalysisAgent) requires the implementer backend to enforce staging contracts while the agent investigates handoff evidence and writes operator drafts.stop_hooksand runsAgentRunSpec.hooksin-session (_AnalysisToolGuard: PreToolUse write confinement, Stop blocking whilerejection.jsonstands).OpportunityAnalysisAgent.__init__with “requires a provider with tool hooks”.Fix
stop_hooksorresumable._AnalysisToolGuardhooks onAgentRunSpec(unchanged)._resume_for_refused_staging_draftsdrives the same Stop refusal loop outside the provider:publish_complete_staged_tasks(..., quiescent_sec=0.0)before each refusal check so a corrected draft clears stalerejection.json._on_stopblocks,backend.resumewith the refusal text, capped by_MAX_STOP_DENIALSand the same analysis deadline as the primary session (remaining budget passed into each resume; stop if the deadline is exhausted mid-recovery).AgentProviderError(and other resume failures) asANALYSIS_STATUS_FAILEDinstead of silently completing.stop_hooks,CODEX_HOME/config.tomlcommand hooks, or a shared hook bridge — those would affect every Codex caller (implementer, lanes, fusion, applyback).Write confinement on Codex continues to rely on existing
sandbox/workspace_guard/protected_paths.Test plan
pytest src/kernelforge/tests/kernel_rewrite_controller/test_opportunity_agent.py(hookless resume republication, deadline cap, resume failure reporting, hooks-or-resumable gate)pytest src/kernelforge/tests/test_lane_session_guarantees.py(Codex lane advisory for missingstop_hooks)ruff check+ruff format --checkon touched files