Security: fail closed when the conversation loop body throws - #487
Open
lezama wants to merge 9 commits into
Open
Security: fail closed when the conversation loop body throws#487lezama wants to merge 9 commits into
lezama wants to merge 9 commits into
Conversation
`WP_Agent_Conversation_Loop::run()` wrapped only the turn-runner call in a try/catch that finalizes the run to STATUS_FAILED, persists the transcript, and emits a `failed` event. The rest of the outer loop body -- post-turn checks, tool-call mediation, message construction, the runtime-tool store, and the caller-supplied `should_continue` continuation policy -- ran under an outer try whose only companion was a `finally` that released the transcript lock. There was no catch. An unguarded `\Throwable` from that region (e.g. `WP_Agent_Tool_Call::normalize`, message construction, the runtime-tool store, or a throwing `should_continue`) escaped `run()` with the run stuck in STATUS_RUNNING forever: no `failed` event, no persisted transcript, and the transcript lock as the only thing the `finally` cleaned up. Fail closed. Factor the existing failure finalization (emit `failed`, finish_run -> FAILED, persist transcript, return the normalized failure result) into a shared `finalize_loop_failure()` helper, and add an outer catch that runs it for any unguarded throw from the loop body, matching the turn-runner boundary exactly. The `finally` still releases the lock. The deliberate turn-runner contract violation (non-array return) is finalized in place and re-thrown to the caller as before; the guard detects it by object identity so it keeps escaping instead of being converted into a structured failure. Adds tests/conversation-loop-fail-closed-smoke.php: a throw from the outer body finalizes to FAILED, emits a `failed` event, and calls the transcript persister (fails without this change); the normal continuation path still completes cleanly; and the non-array contract violation still escapes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
chubes4
marked this pull request as ready for review
August 26, 2026 21:58
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fail closed across the full conversation runtime, not only the provider turn boundary.
pendingruntime requests andreplace_resultpayloads throw before execution, produce zero tool effects, persist failed audit state, and durably fail run control.tool_effect_completedreceipt immediately after execution, while mediation remains responsible for the single outward response.failed, and preserves the original storage throwable as retryable diagnostics.Coverage
Focused coverage verifies public claimed-run lock contention, direct lock contention, malformed pending/replacement policies, policy/runtime-store/hook/truncator throws, effect receipts, approval single-response behavior, contract violations, terminal storage failures, transcript persistence failures, and exactly-once provider/effect execution.
Verification
composer testcomposer phpstangit diff --checkAll checks pass locally. Composer emits host-package PHP deprecation notices before test output, but the repository suite completes successfully.