Repository navigation
feat(pending-questions): hand each decision-card click to exactly one consumer - #5946
Conversation
… consumer The click side of decision cards for owner pending questions, split out of #5942. A card is a HITL `choice` requirement with runtime pending-questions, guard and subject.ask_id = the ask id, and subject.choices = {action id: consequence}; the ask side does not create them yet. `src/pending_questions_reader.py clicks` (and `pq.py clicks`) reaches the adapter's new `clicks` (scripts/pending_questions_clicks.py): - every pass runs under one exclusive lock beside the HITL store; a claim left `claiming` by a pass that died is re-offered (no pid is probed); - the pass claims the click, closes the row as Answered in one guarded write that also requires the owner's Status to be open (RoomDbStore.close require_open), and records the claim `handed` before returning it; the choice prints once with its exact action id and consequence; - later passes, by any consumer, report it as awaiting ack, never as a new choice; `ack <ask-id>` settles it, or marks it acknowledged while its close is retried; - a row already closed by the owner's Status, or by code without this click's claim, makes the click stale and cancels its card; - `resolve` retires the question's card. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/health-check.pyLines 13035-13043 13035 checks.append(_wiring)
13036
13037 _card_clicks = check_decision_card_clicks()
13038 if _card_clicks:
! 13039 checks.append(_card_clicks)
13040
13041 _mem_index = check_memory_index_integrity()
13042 if _mem_index:
13043 checks.append(_mem_index) |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at 6bc14fa (agent review): COMMENT.
These all hold, and each fails its test when reverted:
- the lock;
- the single actionable handoff;
- owner Status beating a late click;
- the exact consequence.
20/20 and 40 neighbouring suites pass, and CI is green.
Blocking: a consumer that dies after the handoff strands the choice for good. If it dies before ack (pending_questions_clicks.py:88-91,110-112), the row is already Answered, later passes print "awaiting ack" (pending_questions_reader.py:233), and CLAUDE.md:171 says to act only on chose. Please add a bounded re-offer, such as a lease or expiry on handed, with a test.
…and clicks fail closed in an outage - A handed choice carries handed_at; one not acked within HANDED_LEASE_SEC (10 min) is re-offered to the next pass with a fresh token, so a consumer that died after the handoff no longer strands it. Exclusive within the lease, at-least-once after it. - `ack` carries the claim token (`--token`); an ack with a superseded token is refused as stale and changes nothing. - The click's row close writes its claim token into the Closed mark (`Answered+<token>@<host>`); a refusal counts as this claim's own earlier close only when the mark carries the same token, so a recovered claim can no longer take credit for an unrelated resolve. - Clicks never close through a local close record: with no room store, an unreachable room, or no row in it yet, the click is deferred (no `chose`, claim kept) until the room can verify the owner's Status is open. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed at a7e6bd4 (agent review): COMMENT.
What's fixed. The stuck handoff: there's now a 10-minute lease, a re-offer with a fresh token, token-bound acks and Closed marks, and outages defer. All 8 reverts fail their tests, 26/26 pass, and CI is green.
Still open: the action itself can run twice. CLAUDE.md:171 says act, then ack. So a consumer slower than the lease lets a second consumer act too, and the refused ack comes after both have acted. A forward wall-clock jump (time.time() at pending_questions_clicks.py:94,104) does the same. Also, the re-offer repeats every 10 minutes, not once.
Two options:
- Accept at-least-once: say so in the PR and the skill doc, and make the click's action idempotent.
- Close it with an ack-to-claim or a renewable lease, using boot id plus monotonic time.
…nd the at-least-once rule - A handed claim past its stored deadline (`lease_until`) is re-offered ONCE (`reoffers`, MAX_REOFFERS = 1). After that it is listed by `clicks` as "NEEDS ATTENTION — handed out twice, never acknowledged", with its token, and never offered again. - The lease compares against the stored deadline, and a `handed_at` in the future (the clock went back) counts as fresh, not expired. - The contract is at-least-once, at most twice: SKILL.md, CLAUDE.md and AGENTS.md say to re-check current state (already merged, already resolved, already done) before acting on a `chose`. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tored deadline Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact current head 689f359f0ac69f08fcb42c951fefae9603889251.
Verdict: CHANGES_REQUESTED — not ready to merge. The bounded single re-offer, future-clock guard, token-bound ack, and ordinary outage paths now pass. One owner-authority failure remains.
Blocking — a terminal owner Status can lose to the matching click token after a partial stale-mark cleanup failure. reconcile_pending() catches a failed stale-mark clear() and continues (store.py#L548-L554); _close_for_click() ignores those reconciliation errors, then its GuardFailed handler accepts a matching Closed token before considering terminal owner Status (room_db.py#L328-L339).
Production-store repro: create the claim-owned Answered+claimtok@host close, then have the owner set Status to Resolved, make only RoomDbStore.clear raise, and run clicks() again:
owner_status=Resolved
actionable=True stale=False token=claimtok
close_message=... closed earlier by this claim
The card was changed to handed, so the obsolete click would be emitted as chose even though owner Status is authoritative. Worst case: the core executes an action the owner explicitly superseded. Please check terminal owner Status before granting a matching token credit, or fail closed on the relevant reconciliation error, and add this cross-product regression. Current tests cover the matching-token recovery and terminal-status outage separately, not together.
Evidence at this head: card-clicks 28/28; room DB 51 passed, 1 optional pycrdt skip; review checks and hardcoded-path scan passed; all 37 check runs and CLA are green. Those green suites currently miss the reproduced interaction above.
…lick-token logic A refused click close now checks the row's owner Status first: Answered or Resolved makes the click stale whatever its Closed mark says. Before, a mark carrying the claim's own token (left when the stale mark could not be cleared) was taken as this claim's close and handed the choice over beside the owner's own answer. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact current head 9794fd11c35bd3445625b82990807b3b4ee57a14 against main at 20f3042b821815ba9e3f191a8d69bedb6282bc56.
Verdict: CHANGES_REQUESTED — not ready to merge. The requested owner-Status ordering fix is correct: the new regression passes, and removing the two Status-first lines makes that exact case fail. Three whole-path issues remain.
-
Blocking — deferred and leased claims have no production wakeup. Lease expiry and deferred retry are evaluated only when
clicks()is called. The only added production trigger is the incominghitl_clicktask; the core is instructed to finish that task with[no-send], and the watcher has a restart sweep, not a timed lease callback. The crash path is therefore:first pass: handed, then dies before print/action restart inside lease: "awaiting ack" (not actionable) task result: [no-send] -> task retired lease expiry: no caller remains -> never re-offeredA transient room outage strands a
deferredclaim the same way. The regression attests/pending-questions-card-clicks.test.py:189-206manually invokesclicks()at a future timestamp, so it proves the state can retry, not that production will retry it. Add a durable delayed wakeup/requeue and test the real task lifecycle through expiry/recovery. -
Blocking — the new room mark is not readable by the reviewed base during rollback/rolling upgrade. New code writes
Answered+<token>@<host>, while only the new parser strips the token. Exact-base/head probe:raw_closed=Answered+0123456789abcdef@test-host base 20f3042b effective_status=Open head 9794fd11 effective_status=AnsweredAfter rollback, a clicked question reappears open and can be reminded again; old code also cannot close it because
Closedis already non-empty. Keep the token in a backward-ignored field or use a transition that remains legible to the old parser, with a base-reader compatibility test. -
Blocking — a newly handed ten-minute lease can already be expired.
nowis sampled before the blocking lock and reused after lock wait plus any earlier card/room work. Controlled timing probe:sampled now=100; lock acquired / handoff at=701 stored lease_until=700 immediate next pass: actionable=true, reoffered=trueSample the clock inside the lock at each handoff/re-offer so the advertised lease begins when that transition is persisted.
Positive evidence: the prior matching-token/terminal-Status bug is fixed (head: actionable=False, stale=True; fix-removed control: actionable=True, stale=False). Focused checks passed 163 tests with one optional pycrdt skip; review-checks.sh, git diff --check, and the hardcoded-path positive control behaved correctly; current CI and CLA are green.
…egible close mark, a lease that starts at the handoff, and no text beside a live card - Wakeup: src/health-check.py (every proactive-loop pass and the launchd fallback) warns `decision-card-clicks` when the core reader's new read-only `clicks --due` probe says a pass is owed (a click never processed, a claim left `claiming` by an outage or a death, or a handoff past its deadline with its re-offer unused), naming `… clicks` to run. - Rollback: a click close writes a plain `Answered@<host>` in Closed and its token in Recovery (`click:<token>@<host>`), which the reviewed base reads as closed and ignores; the token is read back from the row on a refusal. - Lease: the clock is sampled at each handoff and re-offer inside the lock, so lock wait and room I/O no longer shorten a new lease. - Card records are recognised by the reminder here, the direct parent of the ask side: asked_recently treats a live card as the ask, and remind drops live-card questions from any text, --force included, so rolling the ask side back cannot bring a text ask beside a live card. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@keweichen, replying to your review at 9794fd11: all three findings are fixed in place at 6c57b3e. Each new test fails with its fix removed. 1. No production wakeup.
2. The mark isn't readable to the base after a rollback.
3. A new lease can already be expired.
This round also moves the card-record reader ( |
… cause The new health check's name must be in recovery_issues.HEALTH_CAUSES, or its warn leaves the host as an unknown cause; tests/recovery-issues.test.py pins that inventory against the check names in src/health-check.py. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed the current head afe5424861c8d0e60141a84dbfa7e0e43d40c198 against base 20f3042b821815ba9e3f191a8d69bedb6282bc56. APPROVE.
The prior blockers are resolved: due/expired leases now surface through the recurring decision-card-clicks health probe; terminal projection remains readable after rollback while token detail stays in Recovery; and lease time is sampled under the production click lock. The only delta since the previously reviewed head registers decision-card-clicks in the recovery inventory, which fixes the deterministic inventory failure.
Evidence at this head: five blocker-focused regressions pass; tests/recovery-issues.test.py is 12/12; tests/health-check-recovery-telemetry.test.py is 16/16; the relevant card-click suite was 40/40 at the patch-equivalent prior head; mutation checks caught inventory removal and mark-before-status ordering. Current CI and CLA are green (one intentional skip). The remaining model interpretation of a recurring WARN is disclosed and acceptable for this model-driven loop.
|
@qingyun-wu could you give this PR a second review at afe5424? keweichen approved it at that head; the branch rule needs one more approval. You reviewed earlier rounds. What changed since then: owner Status now wins before token recovery; a health-check wakeup retries deferred and expired leases; the Closed mark stays readable after rollback; and the lease deadline is set after the lock is taken. Each fix has a test that fails without it. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed at afe5424 (agent review). The suite passes 40/40, each revert fails its test, and CI is green.
Verified:
- one re-offer, then NEEDS ATTENTION;
- health-check's
decision-card-clickswakeup; - owner Status is checked before the token logic;
- the at-least-once rule is documented.
Still open:
pending_questions_clicks.py:98keeps a future-datedhanded_atfresh, so a clock that was ahead holds the click for the whole skew. I reproduced it: with the clock a year ahead, the click was still stuck at +30 days.due_clicks(:157) skips that case and NEEDS ATTENTION, so health-check never surfaces either one.
Suggested fix: clamp the future stamp, and include both cases in due_clicks.
…from first sight; due_clicks surfaces it and NEEDS ATTENTION A handed_at ahead of this clock is still never expired on sight, so a backward step cannot steal a live lease; but the first pass that sees it records skew_seen_at, and it expires one lease after that, so a clock that ran far ahead no longer holds the click for the whole skew. A stored deadline beyond one lease is clamped to handed_at + lease. due_clicks (and so the health check's decision-card-clicks warn) now also surfaces an unseen skewed claim and a NEEDS ATTENTION one. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@qingyun-wu, replying to your review at afe54248: both points are fixed at d9f49af, reconciled with keweichen's rule that a backward clock step must not steal a live lease. 1. A future-dated
2.
Tests: card-clicks 44/44, and every pending-questions and hitl suite plus |
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head d9f49af5 against base 20f3042b. CHANGES_REQUESTED — not merge-ready.
The earlier blockers remain resolved, and the far-ahead single-generation case works. The new skew_seen_at state is not scoped to the full current lease generation, however, which leaves two adjacent failures in the production writer.
Blocking — the first-sight bound is dropped when the clock catches up. _lease_state() consults skew_seen_at only while handed_at > now; afterward it returns to the original handoff deadline. With L=600, handed_at=T+599, and first observation at T, real clicks() records skew_seen_at=T without acting. At T+600 it is still non-actionable and due_clicks() is empty; it re-offers only at T+1199. That is 1,199 seconds after first sight, nearly two leases rather than the documented one.
The old observation also leaks into the fresh re-offer. The spread at lines 139–146 retains skew_seen_at. In an exact writer probe, a far-future handoff was first observed at T, then re-offered late at T+700; a one-second rollback to T+699 immediately produced needs_attention=True for that brand-new 600-second lease. A control that cleared the old marker recorded a new first sight and did not raise attention.
Please bind/reset the observation marker for each handoff/re-offer and keep the first-sight deadline effective even after the local clock passes handed_at. Add both regressions: a future offset of L-1, and a delayed re-offer followed by a one-second rollback. Worst case today is an extra lease of stranded recovery in one direction, or an immediate false persistent health warning and premature owner intervention in the other.
Evidence: exact-head card-click suite 44/44; all 42 focused pending-question/HITL entrypoints passed with two optional skips; exact-head CI and CLA are green; git diff --check and the review scanner are clean, with a firing hardcoded-path positive control. Those tests cover the submitted far-ahead and naive-expiry mutants but not these generation-boundary cases.
…r, not a wall-clock deadline Replaces handed_at/lease_until/skew_seen_at with one invariant (scripts/pending_questions_lease.py): a lease expires exactly when the time credited to it reaches its window, and each pass credits clamp(now - observed_at, 0, max_step), where observed_at only moves forward. A backward step credits nothing and catching back up credits nothing (no span counted twice), so no rollback can expire a lease or raise NEEDS ATTENTION early; a forward jump credits at most LEASE_MAX_STEP_SEC (300 s, the health check's fallback cadence); only deltas between passes count, never a stamp's distance from this clock; a handoff or re-offer starts a new lease. Every clicks pass and every clicks --due (the health check) credits and saves the lease atomically under the clicks lock, so a restart resumes from the saved credit. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@keweichen, replying to your review at d9f49af5: instead of a third boundary patch, the deadline and skew-marker logic is replaced by one invariant, at 58b4de3 ( Invariant. A lease expires exactly when the time credited to it reaches its window. Each pass credits Consequences:
The trade-off I chose. Your second finding (a 1-second rollback after a late re-offer). The re-offer resets the lease, and the rollback credits 0. Result: Tests:
card-clicks 45/45, plus every pending-questions and hitl suite and |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 58b4de39e887421e2c9719626e5bb86e89624f9c against base 20f3042b821815ba9e3f191a8d69bedb6282bc56.
CHANGES_REQUESTED — not merge-ready. The renewal reset, rollback/catch-up, and persisted restart-credit cases requested last round now work, but two production transitions still return the wrong liveness/authority result.
-
Blocking — a forward clock spike followed by correction freezes recovery for the entire spike.
advance()caps the credited delta at 300 seconds but persists the full future wall value asobserved_at. Every corrected pass below that high-water then credits zero. An exact productiondue_clicks()trace from a fresh handed claim produced:pass at T+1 year: elapsed=300 due=False clock corrected to T: elapsed=300 due=False corrected T+600: elapsed=300 due=False corrected T+1 day: elapsed=300 due=False T+1 year+300: elapsed=600 due=TrueSo a transient forward spike can strand a dead consumer's choice—with no re-offer or health warning—for a year (or any size spike). This regresses the prior far-ahead first-sight bound and contradicts the module's “neither … stretch” / “stamp distance never matters” contract. The test named
test_a_far_future_stamp_does_not_mattersetsobserved_at=t, not a future stamp, so it cannot catch the failure. The submitted W−1 case also still expires at first sight + 1,199 seconds, exactly the trace requested last round. Please retain a per-generation bound from first local sight (or another restart-safe bounded clock design) and add true future-stamp plus forward-spike→correction production regressions. -
Blocking — a claim save rejected because the card became terminal still returns an actionable decision.
stamp_claim()correctly reloads under the HITL store lock and skips a terminal card, but the handoff and re-offer callers ignore whether that transition landed. Meanwhileresolve()retires the card without the clicks lock. Deterministically injecting productionresolve()immediately before each save gave this at both initial handoff and expired re-offer:resolve: decision card … retired card_status=cancelled actionable=TrueOn re-offer, the fresh returned token was not persisted; the terminal card retained the old token. Worst case, the core executes a choice after the owner has terminally resolved the question, and the resulting token cannot even be acknowledged. Make the transition report whether it committed/current state and emit no actionable result after a rejected save (or serialize the whole authority transition), with resolve-vs-handoff and resolve-vs-re-offer regressions.
Evidence at this head: card-click suite 45/45; the three advertised naive lease mutants are caught (6 methods/12 subtest failures, 6/14, and 1/3 respectively), but none exercises spike→correction; review scanner passed with a firing hardcoded-path control; git diff --check is clean; all current CI and CLA checks are green. Those green checks miss the two traces above.
…ezes a lease; a retired card wins the claim save
- Lease: each pass credits clamp(now - observed_at, 0, max_step) and then
sets observed_at = now. A spike of any size credits at most one step and
normal time resumes once the clock is corrected (it no longer freezes the
lease for the whole spike). The trade-off, stated in the module: catching
back up after a rollback is credited again, a bounded over-credit of at
most one step, never an unbounded freeze. A rollback still credits 0 and
no single pass expires a fresh lease (max_step < window).
- stamp_claim reports whether the save landed; it is refused for a card a
concurrent resolve() retired. The handoff, the re-offer and the first
claim then hand nothing over, return no token, and re-read the card
("settled meanwhile — not a decision").
Stand: Echo Act IV Pro
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@keweichen, replying to your review at 58b4de39: both findings are fixed at 6cf31ae. Each new test fails on 58b4de3. 1. A corrected forward spike froze recovery.
This reverses your earlier "no double credit" pin, deliberately. Refusing all double credit means keeping Tests:
2. A retired card still produced an actionable decision.
card-clicks 50/50; every pending-questions and hitl suite, plus |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6cf31ae9e888bdb2429173c2475c4d132dc9df92 against base 20f3042b821815ba9e3f191a8d69bedb6282bc56.
APPROVE for code. The prior two blockers are resolved. This PR is not merge-ready until the required CI rerun is green.
The shared lease now credits clamp(now - observed_at, 0, max_step) and records this pass's now, so a corrected forward spike resumes bounded progress. The production suite covers W−1/W/W+1, repeated timestamps, rollback/catch-up, a year spike followed by correction, a true future stamp, renewal, and persisted restart state. The deliberate downside is bounded catch-up over-credit: repeated clock oscillations can accumulate credit, but each pass is capped below the window and cannot expire a fresh lease alone. That tradeoff is documented and acceptable.
The terminal race is also fixed: the store-locked claim save reports whether it landed after re-reading the card, and initial handoff/re-offer expose neither an actionable result nor a token when production resolve() wins. Both injected races leave the card terminal; re-offer retains only the old persisted token.
Evidence at this head:
tests/pending-questions-card-clicks.test.py: 50/50- Seven current regressions run against prior head
58b4de39: 11 failures, covering spike correction, W−1 recovery, and refused-save races - High-water
observed_atmutant: 11 failure records - Review scanner and
git diff --check: clean
I read the current red CI. python standalone tests (2/9) timed out tests/hermetic-env-structural.test.py after 240 seconds; it did not report a product assertion failure, and the aggregate gate is red only as a consequence. That exact test passes locally 27/27 in 61s, and the same shard passes on cumulative child #5947. This looks like runner timing rather than a diff defect, but required CI still needs a green rerun before merge.
Nonblocking documentation nit: src/pending_questions_reader.py / the skill still call clicks --due read-only even though it intentionally persists lease credit.
|
CI at 6cf31ae (run 40505837004): shard 2/9 failed on a timeout, not an assertion: I believe it's a runner-speed flake, not this PR:
I re-ran the failed jobs without pushing, so the approved head is unchanged. The re-run passed, and run 40505837004 is now |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at 6cf31ae (agent review). The suite passes 50/50 and CI is green.
Verified by repro:
- The year-ahead handoff re-offers after 3 passes (15 min).
- A clock set back an hour never makes a live lease actionable.
- A corrected spike costs one step.
- A resolve injected at the handoff leaves the click unactionable and marked retired.
Non-blocking: over-credit is bounded per rollback, not in total. A ±1000 s flapping clock expired a live lease in 4 passes (4 s). The at-least-once rule covers it; a cap would tighten it.
…lt or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt or --option is given (#5947) * feat(pending-questions): ask-owner asks by decision card when --default or --option is given The ask side of decision cards, stacked on the click side (#5946), split out of #5942. - src/hitl/cards.py: one card builder (choice_request, create_choice, manager_for, new_id); report-feedback and the worker pool's wedge cards build through it, and the clicks module takes its store handle from it. - queue_question builds the card with a pre-allocated id, holds the question (outbox, then the row) naming it, and only then stores the card. Once a card exists no text is sent. create_card returns stored / not stored / unknown (or no card); stored and unknown keep the card and report that no text was sent. - A definite card failure texts the question to its own destination with the row's own link (the one the row write returned), then holds the question again flagged `redeliver`; a landed row is rewritten now (RoomDbStore.redeliver) and the entry released, and an unrewritten or unconfirmed row takes the record at the next reconcile (Outbox.flush). - The reminder never texts a question whose card is live, --force included; recency is the gateway's notified_at once posted; a card unposted after 30 min gets one text pointer, once per card, with no options. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): claim the card pointer before publishing it; commit history before releasing a fallback hold - send_card_pointers now claims each pointer first (claim_pointer: under the HITL store lock, re-check the card is live, unposted, past the grace and unclaimed, then stamp pointer_at with the store's fsynced save) and only the claimant publishes. Overlapping reminders queue one pointer; a death after the claim queues none; a card posted before the claim gets none. - The definite-failure fast path now commits store history, then rewrites the landed row, then releases the redeliver hold (the order Outbox.flush keeps). A history that cannot be written keeps the hold, so a later room outage lists the question instead of a measured zero. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): a card pointer is claimed, published, then marked sent; a stale claim is retried The pointer state is split: claim_pointer stamps pointer_claim_at and a token under the HITL store lock, the claimant publishes, then mark_pointer_sent records pointer_sent_at. A claim with no sent mark older than POINTER_CLAIM_RETRY_SEC (5 min) — a claimant that died or failed to publish — is claimed again and retried. The pointer is now at-least-once (a death between the publish and the sent mark can deliver it twice), and SKILL.md says so. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): a pointer is re-checked before it is written, never revives a settled card, and a back-stepped clock cannot steal its claim - send_card_pointers re-checks the card under the store lock just before writing the file (live, unposted, still this claim's) and writes it inside that lock, so a post recorded after the claim cancels the pointer. The window left is a projector send in flight (it records the post only after its send returns): bounded by one send, documented. - mark_pointer_sent refuses a terminal record, so a card settled or archived after the claim is never written back live. - A pointer claim stamped in the future (the clock went back) stays fresh until its window passes, so a second claimant cannot take it. - The card-record reader (card_sent / card_state / card_live) now lives in the parent (#5946); this layer keeps the pointer and sent_at logic. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): a pointer claim stamped ahead of this clock holds one window from first sight Same bound as the click lease: a pointer_claim_at ahead of this clock is never taken on sight (a backward step cannot steal a fresh claim), but the first reminder that sees it records pointer_skew_seen_at and the claim can be retried one POINTER_CLAIM_RETRY_SEC after that, so a clock that ran far ahead no longer suppresses the pointer for the whole skew. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): a pointer claim uses the same elapsed-time lease as a click Replaces pointer_claim_at / pointer_skew_seen_at with the shared lease (pending_questions_lease): a pointer claim is kept until its lease has been credited POINTER_CLAIM_RETRY_SEC across reminder passes, each pass crediting clamp(now - observed_at, 0, POINTER_MAX_STEP_SEC) with observed_at only moving forward; a new claim starts a new lease. No clock step replaces a live claim, and a near- or far-future stamp holds nothing beyond the observed window. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(pending-questions): pin the pointer claim lease under rollback over-credit and a corrected year spike The parent's lease now sets observed_at = now each pass (a corrected forward spike resumes normal time instead of freezing); the pointer claim table pins one step, W-1, a 1 s rollback, the bounded catch-up over-credit, a renewal, a year's spike then correction, and recovery after it. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): every reminder pass credits a pointer claim's lease; grace only gates the replacement claim_pointer returned before crediting an existing claim when the card was still inside its grace, so passes before grace never counted. It now credits and saves the existing lease on every pass first, returns without replacing while still before grace, and mints a replacement only when the lease has expired AND the card is grace-eligible. Also: `clicks --due` is described as writing lease credit (not read-only), and the trailing blank line at the end of src/hitl/cards.py is gone. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(pending-questions): send the unposted-card pointer on every pass, flagless included The 30-minute text pointer for a decision card the gateway never posted ran only under --notify, which is on demand, so an unattended host never sent it. The pointer completes the question's one ask rather than reminding, so it now runs on every pass of src/check-pending-questions.py (the proactive loop's flagless pass included), outside presenter mode; text reminders still need --notify. Docs (CLAUDE.md/AGENTS.md, SKILL.md, proactive-loop, rationale) say so, and the crash-before-hold text is now attributed to the next --notify run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(proactive-loop): keep step 2 under the skill byte cap Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(pending-questions): replace source-text assertions with behaviour tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Stack
PR 1 of 2, split from #5942 (kept as a draft for reference). This PR covers the click side only, based on main. PR 2 (the ask side: creating cards and delivering the question) stacks on this one.
What changed, and why
A pending question's decision card is a HITL
choicerequirement:pending-questionssubject.ask_idset to the ask idsubject.choicesmapping each action id to its consequenceThe bridge owner-checks a click, records it, and forwards it to the core as a task headed
hitl_click: true. This PR hands each recorded click to one consumer at a time, under a 10-minute lease, with at most one re-offer. In this PR, cards come only from a test-only path; the ask side does not create them yet.python3 src/pending_questions_reader.py clicks(orpq.py clicks) reaches the adapter's newclicks, inskills/pending-questions/scripts/pending_questions_clicks.py:One lock, no pid probe. Every pass runs under one exclusive lock beside the HITL store,
state/hitl/pending-questions-clicks.lock. Whoever holds the lock is the only live claimant. A claim still markedclaimingwas left by a pass that died, and is retried with the same token.Token-bound close. The pass claims the click with a token, saved atomically on the card. It then closes the row as Answered in one guarded write that:
Answered@<host>in Closed, and the token in Recovery (click:<token>@<host>). A reader that predates tokens, such as the reviewed base after a rollback, reads the row as closed and ignores Recovery.The owner's terminal Status is checked first: Answered or Resolved makes the click stale, whatever any mark says. Otherwise, a refusal counts as this claim's own earlier close only when the row's Recovery token is the same. Any other refusal makes the click stale and cancels its card. That covers the owner's own Status and a close by code such as
resolve, so a recovered claim cannot take credit for an unrelated resolve.Fail closed. Only the room can say the owner's Status is open, so a click never closes through a local close record. The click is deferred when there is no room store, when the room is unreachable, or when the row is not in the room yet. Deferred means no
chose, the claim kept, and a retry on the next pass.Leased handoff. On a close, the claim is recorded
handedwithhanded_atbefore the choice is returned.[<ask-id>] chose <action-id> '<label>' -> <consequence> … ack with --token <token>.pending_questions_lease.py, shared with feat(pending-questions): ask-owner asks by decision card when --default or --option is given #5947's pointer claim). Invariant: a lease expires exactly when the time credited to it reaches its window (HANDED_LEASE_SEC, 600 s). Each pass creditsclamp(now - observed_at, 0, LEASE_MAX_STEP_SEC=300)and then setsobserved_at = now. Consequences:observed_atat its high water and freezes the lease for the whole span of a corrected spike, which is unbounded.clicksand everyclicks --due(the health check) credits and saves the lease atomically under the clicks lock, so a restart resumes from the saved credit.choseto the next pass, once, with a fresh token and a new lease. A consumer that died after the handoff therefore no longer strands the choice.clicks --duesurfaces it.chose. SKILL.md, CLAUDE.md and AGENTS.md now carry the rule: before acting on achose, re-check the current state (already merged, already resolved, already done) and skip what is.Token-bound ack.
… ack <ask-id> --token <token>settles the card. An ack carrying a superseded token, from a consumer that acked after its lease was re-offered, is refused as stale and changes nothing.Wakeup.
src/health-check.pyruns on every proactive-loop pass and from the launchd fallback. It warnsdecision-card-clickswhen the read-only probeclicks --duereports a pass owed: a click never processed, a deferred or interrupted claim, or an expired handoff whose re-offer is unused.… clicks, which the core handles as for ahitl_clicktask. So a deferred click or an expired lease is retried with no new task.Lease clock. The clock is sampled at each handoff and re-offer, inside the lock, so lock wait and room I/O never shorten a lease.
No text beside a live card. The reminder recognises a decision-card record.
asked_recentlytreats a live card as the question's ask, andreminddrops live-card questions from any text,--forceincluded. This PR is the direct parent of the ask side (feat(pending-questions): ask-owner asks by decision card when --default or --option is given #5947), so rolling that back cannot bring a text ask beside a live card.A retired card wins. Each claim save re-checks the card under the HITL store lock and is refused for a card a concurrent
resolvehas retired. The pass then hands nothing over and returns no token ("settled meanwhile — not a decision").resolveretires the card. It cancels the question's card whenever it closes the row or the room refuses the close.Docs: CLAUDE.md and AGENTS.md (Pending decisions), and the skill's SKILL.md ("Decision-card clicks").
Evidence
All tests are in
tests/pending-questions-card-clicks.test.py(50 tests, OK). The crash and concurrency tests use real processes (os._exit) against the HITL store and the room database, through the adapter's own serve over the fake room's on-disk state.Each item below fails with its fix removed:
choseline appears across two processes. With replay-until-ack put back, it fails2 != 1.handed_at: treated as fresh, including when the stored deadline has passed. With the guard removed, the test fails.claiming; a real process runsresolve(Answered)and dies while retiring the card. The retried click is stale and neverchose. With any code close accepted as the claim's own, it fails.chose. With the Status-first check removed, the test fails.chose … (re-offered. A click deferred in an outage is handed over the same way.effective_statusreads a clicked row asAnswered. With the token written into Closed, the test fails.{701, 0}.resolve()injected before the handoff save, and before the re-offer save. Nothing actionable, no unsaved token.due_clicks()spike trace:(300, not due), then corrected(300, not due), then(600, due).--forceincluded. With the filter removed, the test fails.claiming, and no local close is written. Once the room is back, the click is stale. With a local fallback put back, the test fails.Also covered, as before:
resolveretires the card;--tokenrequired, and a stale ack exiting 1) andpq.pyforwarding.Tests run
tests/pending-questions-card-clicks.test.py, 50 tests, OK.health-check-fix-down-bridgesandhealth-check-recover-cronfail the same way on the unchanged parent on this host; they depend on host state.pending-questions-*suite, andtests/skills/pending-questions/pq.test.pyhitl-*suiteagent-api-pending-questions,check-pending-questions-collapseandfriction-pending-questions-collapsehermetic-env-structuralrun_all_checks; its function is covered.scripts/review-checks.shpasses, andagents-md-sync.sh --checkpasses.Feature/documentation consistency
docs/catalog.jsonhas no canonical document for pending questions. The behaviour is documented in SKILL.md and CLAUDE.md, as listed above.Live path
No live round trip has been run: no card is created on main until PR 2.
🤖 Generated with Claude Code