feat(conch): add awaiting_human_response priority-override state - #501
Open
GrowDev1 wants to merge 3 commits into
Open
feat(conch): add awaiting_human_response priority-override state#501GrowDev1 wants to merge 3 commits into
GrowDev1 wants to merge 3 commits into
Conversation
Adds a new conch lock state, `awaiting_human_response`, for the case where a voice-conversation turn is blocked on a pending human confirmation (e.g. an agent asking the user to approve a risky action) rather than an ordinary between-turns hold. Unlike a normal hold, this state: - Is a PRIORITY OVERRIDE in `_held_by_other()`: it blocks all other processes' floor-acquisition attempts unconditionally, even if the original holder's pid has died -- cleanup on drop/crash is the caller's job (e.g. an approval-queue's own expiry), not a pid-liveness check here. - Is exempt from the normal `CONCH_HOLD_EXPIRY` idle-refresh window, since a real human decision can take much longer than a between-turns refresh; it carries its own explicit `response_deadline` instead. - Exempts the ORIGINAL holder's own process (verified via pid + process-start-time, not a bare pid comparison, since pid alone is not a reliable process identity across process death/reuse) so it can resume the instant the human responds. New `Conch` classmethods: `mark_awaiting_human_response()` (enter the state), `awaiting_human_response_active()` (the correct, unconditional way for a caller to check whether the state is currently blocking -- deliberately not built on `get_holder()`/`is_active()`, which depend on pid-liveness and the unrelated `CONCH_LOCK_EXPIRY` staleness window), and `resolve_awaiting_human_response()` (explicit resume path once the human has actually answered). `_check_and_clear_stale_lock()` gets a matching guard so its existing dead-pid fast-fail path can't silently clear a live `awaiting_human_response` marker out from under this new mechanism. In `converse()` (`_converse_core`), `skip_conch=True` is now a hard no-op while `awaiting_human_response_active()` is set -- it must never be possible to talk over a pending safety confirmation, no matter what the caller passed. A payload with no `awaiting_human_response` key at all (every existing lock file today) hits none of the new branches -- fully backward compatible with the current two-state (`held` True/False) shape. Note: `_process_start_time()` reads `/proc/<pid>/stat` for process identity and is therefore Unix-only; on Windows it returns None, which `_is_same_process()` correctly treats as "not confirmed self" (fails closed) -- so the same-process resume exemption doesn't fire on Windows today, though the new state's core blocking behavior is unaffected there. Flagging for discussion given this file's own recent Windows-portability work (fcntl -> file_lock, os.kill -> psutil.pid_exists) -- a psutil.Process(pid).create_time()-based identity check would close that gap and is worth a follow-up if wanted.
_process_start_time() previously read Linux-only /proc/<pid>/stat, returning None (fail-closed) on every other platform -- including native Windows, which the rest of this file was already made portable for (psutil.pid_exists, voice_mode.file_lock instead of fcntl/os.kill). That meant the same-process resume exemption in _is_same_process() silently never fired outside Linux: the very process that called mark_awaiting_human_response() could never resume its own turn, exactly the HIGH-severity bug the round-2 audit fix was built to prevent, reintroduced on non-Linux platforms by the round-3 pid-reuse-protection fix. Caught by running the ported test suite below natively on Windows, not assumed from the prior commit's own documented "flagging for discussion" note. Switches to psutil.Process(pid).create_time() -- an epoch-seconds float, stable and portable, matching this module's existing psutil-based liveness probes elsewhere in the file. Adds tests/test_conch_awaiting_human_response.py (34 tests) ported from the original design/validation suite, adapted to this repo's current conch.py/converse.py structure: real subprocess pids instead of a hardcoded pid=1 (not guaranteed to exist on Windows), imports the installed voice_mode.conch package directly, and drops the Thessary-internal approval_queue composition checks and the original-vs-patched bug-repro comparison, neither of which apply to this standalone repo. Confirmed zero regressions: all 224 existing conch/converse-related tests plus these 34 pass together (258/258).
An independent review found the comments/docstrings from the prior two commits leaked references that only make sense in a private downstream project: an internal design-doc path, internal theme labels, a named downstream project and a file that doesn't exist in this repo (approval_queue.py), and an "independent audit round N" review-process narrative. Rewrote every affected comment/docstring as plain, neutral API documentation describing the WHY of each decision without any of that context -- no behavior change. Also adds a test for calling mark_awaiting_human_response() a second time while already in the awaiting state (extends the deadline cleanly, preserves identity fields, no corruption) -- a real gap the same review pointed out, and removes an unused `import pytest` from the test file.
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
1 participant
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
Adds a new Conch lock-file state,
awaiting_human_response, for the case where a voice-conversation turn is blocked on a pending human confirmation (e.g. an agent asking the user to approve a risky action) rather than an ordinary between-turns hold.Unlike a normal hold, this state:
_held_by_other(): it blocks all other processes' floor-acquisition attempts unconditionally, even if the original holder's pid has died — cleanup on drop/crash is left to the caller (e.g. an approval-queue's own expiry), not a pid-liveness check here.CONCH_HOLD_EXPIRYidle-refresh window, since a real human decision can take much longer than a between-turns refresh; it carries its own explicitresponse_deadlineinstead.New
Conchclassmethods:mark_awaiting_human_response()(enter the state),awaiting_human_response_active()(the pid-liveness-independent way to check whether the state is currently blocking), andresolve_awaiting_human_response()(explicit resume path for an early "yes" rather than waiting out the full deadline)._check_and_clear_stale_lock()gets a matching guard so its existing dead-pid fast-fail path can't silently clear a liveawaiting_human_responsemarker out from under this new mechanism. Inconverse(),skip_conch=Trueis now a hard no-op whileawaiting_human_response_active()is set — it must never be possible to talk over a pending confirmation, no matter what the caller passed.A payload with no
awaiting_human_responsekey at all (every existing lock file today) hits none of the new branches — fully backward compatible with the current two-state (heldTrue/False) shape.Note on current scope:
mark_awaiting_human_response()/resolve_awaiting_human_response()have no caller in this repo yet — this PR adds the primitive and its guard-side enforcement (theconverse()skip_conch check), for a caller (e.g. an external approval/confirmation flow) to invoke going forward. Happy to adjust the framing or narrow this PR if you'd rather see it land alongside an actual call site.Cross-platform fix included: the process-identity check backing the "original holder can resume" exemption was drafted against
/proc/<pid>/stat(Linux-only) and silently failed closed on every other platform — including Windows, which the rest of this file already supports viapsutil/voice_mode.file_lock. Switched topsutil.Process(pid).create_time(), which is portable and matches this module's existing liveness probes.Verified no overlap with VM-1967 (this branch is on top of it) — the new code touches a different guard than the
ConchQueuedeadlock fix.Test plan
tests/test_conch_awaiting_human_response.py(35 tests) covering: normal hold/acquire regression, the new state's block/exempt/expire semantics, theconverse.pyskip_conch guard (extracted from real source and exercised directly), pid-reuse protection, and the resolve-guard's precondition.test_conch.py,test_conch_cli.py,test_conch_mcp.py,test_conch_notify.py,test_conch_queue.py,test_converse_conch_queue.py,test_converse_skip_conch.py,test_vm1967_conch_deadlock_repro.py— 259/259 total, run locally on native Windows (not just WSL, confirming the cross-platform fix).