Skip to content

feat(pending-questions): ask-owner asks by decision card when --default or --option is given - #5947

Merged
sonichi merged 11 commits into
mainfrom
feat/pq-card-ask
Oct 28, 2026
Merged

sonichi merged 11 commits into
mainfrom
feat/pq-card-ask

Conversation

@sonichi

@sonichi sonichi commented Oct 27, 2026 •

Copy link
Copy Markdown
Owner

Stack

PR 2 of 2, split from #5942 (kept as a draft for reference). It stacks on #5946 (the click side): base it on, and merge it after, feat/pq-card-clicks. It is rebased onto #5946 at 6cf31ae. That head adds the leased handoff (one re-offer at most), the token-bound ack, the owner's Status winning, the health-check wakeup, the rollback-legible close mark, and the card-record reader that never texts beside a live card. This is the ask side: creating cards and delivering the question.

What changed, and why

scripts/ask-owner.py --default/--option sent plain text only. With this PR, a question bound for AG2 Space is asked as a decision card. The card is a HITL choice requirement in the exact shape #5946 consumes.

  1. One card builder: src/hitl/cards.py. It provides choice_request, create_choice, manager_for and new_id. report-feedback and all four worker-pool wedge cards now build through it, and the clicks module from feat(pending-questions): hand each decision-card click to exactly one consumer #5946 takes its store handle from it.

  2. Order.

    • queue_question builds the card with a pre-allocated id.
    • It holds the question in the outbox; the adapter then writes the row. Both name the card id.
    • Only then does create_card store the card. So no card exists without a durable question.
    • The ask reports stored, not yet shown: decision card hitl_<id>.
  3. One surface. Once a card exists, no text is sent for that question. create_card returns one of four outcomes:

    • stored, including a store that raised after the card file was written;
    • unknown, when the store cannot be checked;
    • not stored;
    • no card, for a question asked by text.

    stored and unknown both keep the card. The report says no text was sent and never says "asked as text" (finding Add context drop setup instructions #5).

  4. Definite card failure (finding README: inline demo video #4). The text goes to the question's own destination.

    • Link: it uses the link the row write returned, not the one guessed before the write.
    • Hold: the question is then held again, flagged redeliver, naming that text file.
    • Rewrite, in order: for a landed row, the store history is committed first; then the row is rewritten (RoomDbStore.redeliver, one body write); only then is the held entry released. This is the same order Outbox.flush keeps.
    • If history can't be written: the hold stays, so a later room outage lists the question instead of reporting a measured zero.
    • Unrewritten or unconfirmed row: it takes the new record at the next reconcile, which replays the entry and rewrites the row before releasing it.
    • Crash: a death before the hold leaves the row naming a card that doesn't exist. The reminder reads that as due and texts again, so the owner is never left without the question.
  5. The reminder (finding Add demo video to README #1).

    • It never sends text for a question whose card is live (pending or clicked), or whose store cannot say, --force included.
    • Once the card is posted, the question's last-asked time is the gateway's notified_at.
    • Pointer for an unposted card: a live card with no post recorded 30 minutes after it was stored gets a text pointer, at-least-once.
      • Claim: under the HITL store lock, the card is re-checked (live, unposted, no pointer sent, no fresh claim) and a claim is stamped (pointer_claim_at, with a token).
      • Publish, then mark: only the claimant publishes the file, then stamps pointer_sent_at.
      • Retry: the claim carries the same elapsed-time lease as a click (feat(pending-questions): hand each decision-card click to exactly one consumer #5946's pending_questions_lease), with window 300 s and step 150 s. A claim whose lease is spent with no sent mark belonged to a claimant that died or failed to publish, so it is claimed again with a new lease and retried. No single clock step replaces a live claim: a rollback or a corrected spike moves the lease by at most one 150 s step.
      • Consequence: overlapping reminders queue one pointer per claim. A death between the publish and the sent mark can deliver it twice.
      • Re-check before writing: the card is re-checked under the store lock just before the file is written. A post recorded after the claim cancels the pointer. The window left is a projector send still in flight, bounded by one send.
      • Sent mark: marking a pointer sent never touches a settled or archived card.
      • Remaining window: a post landing after the claim but before the file is written can still meet a pointer saying the card may not have arrived.
      • Content: the pointer quotes the title and has no options and no approve wording.
    • Nothing is scheduled. All of this happens when the reminder runs.

Docs: SKILL.md ("Decision cards (the ask)"), CLAUDE.md and AGENTS.md (Pending decisions), and the scripts/ask-owner.py docstring.

Evidence

Before and after, from one probe run against an origin/main checkout and against this head: queue_question("Merge #1?", default_action="merge it", options=[("Hold", "wait")]) with the owner last active on ag2space, followed by create_card at HEAD.

== origin/main
proactive files: 1
hitl cards: 0
sent line: **Sent:** queued owner-dm (last-active bridge) via proactive-ask-1793081489311-43762-db8e7a.txt
== HEAD
proactive files: 0
hitl cards: 1
sent line: **Sent:** card hitl_a35fe0cc8848 for ag2space owner-dm (decision card)

tests/pending-questions-decision-card.test.py has 59 tests, all OK. The crash tests use a real os._exit in a subprocess, against the outbox, the HITL store and the room database through the adapter's own serve.

  • Death while holding the question: no card exists.
  • Death after the row, before the card: the row names the id, and the question is due for text.
  • Death between the fallback text and its hold: the question is due, so the text goes again; it is never silent.
  • Death after the hold, before the row rewrite: the next reconcile rewrites the row and releases the entry.
  • Death after the pointer claim, before the publish: not retried inside the 5-minute claim window. After it, the pointer is retried, and exactly one pointer is delivered across all retries.
  • A real process whose pointer publish fails: the pointer is retried once the claim is old. With the retry removed, both of these tests fail.
  • Two overlapping reminder processes: exactly one pointer is published. With the claim stamp removed, the test fails.
  • A card posted just before the claim: no pointer.
  • A card posted after the claim, before the write: no pointer. Without the re-check, the test fails.
  • Claim → settle → archive → mark: the card is not revived. Without the terminal check, the test fails.
  • Pointer-claim lease table: one step; W−1; 1 s back; the catch-up over-credit, bounded by one step; W gives a new claim; 1 s back after the renewal keeps it; a year's spike then correction, then recovery. A claim stamped just under a window ahead is replaced one window after first sight, never on sight.
  • Fallback with the history markers unwritable: the hold stays, and an offline gather is never a measured zero. The next reconcile rewrites the row and releases the hold. With history-first removed, the test fails.

Each finding's test fails with its fix removed:

Also covered:

  • a long label, duplicate labels and options-only buttons;
  • the pointer: sent after the grace period and not before; two passes produce one pointer; never sent for a posted or settled card; no option wording;
  • a mixed reminder list texts only the questions without a live card.

Tests run

tests/pending-questions-room-db.test.py's source-order pin now checks two orders: text is queued then held, and a card's question is held before the card is stored.

Live path

No live round trip has been run; a restart for it needs the owner's go-ahead. The bridge is untouched, and the card goes through the existing _project_hitl and HitlReplyHandler path.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • skills/pending-questions/scripts/pending_questions_ask.py (99.5%): Missing lines 188
  • skills/pending-questions/scripts/pending_questions_clicks.py (100%)
  • skills/pending-questions/scripts/pending_questions_outbox.py (100%)
  • skills/pending-questions/scripts/pending_questions_remind.py (100%)
  • skills/pending-questions/scripts/pending_questions_room_db.py (100%)
  • skills/pending-questions/scripts/pending_questions_store.py (100%)
  • skills/report-feedback/report-feedback.py (100%)
  • skills/worker-pool/scripts/pool_wedge_cards.py (100%)
  • src/hitl/cards.py (100%)

Summary

  • Total: 273 lines
  • Missing: 1 line
  • Coverage: 99%

skills/pending-questions/scripts/pending_questions_ask.py

Lines 184-192

  184 def _pointer_owed(manager, card_id: str, token: str):
  185     """The live, unposted card this claim still holds; None otherwise. Call under the store lock."""
  186     req = manager.get(card_id)
  187     if req is None or req.terminal or (req.subject or {}).get("pointer_claim") != token:
! 188         return None
  189     return None if manager.store.projection(card_id).get("revision", 0) > 0 else req
  190 
  191 
  192 def mark_pointer_sent(manager, card_id: str, token: str, now: float) -> None:

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at b179034 (agent review). Both earlier gaps are closed:

  • Unposted cards: one gets a single no-options pointer after 30 min, recorded as pointer_at.
  • Card failures: a definite failure texts the original destination with the row's link, re-holds the question as redeliver, and reconcile rewrites the row.

Each of the 10 fixes fails its test when reverted. 48/48 pass, and so do 50 neighbouring suites.

Nit: the asked_recently docstring (pending_questions_ask.py:238) still credits HITL renotify.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 9188fce (agent review). The range-diff against b179034 shows only the base's ack wording and the asked_recently docstring fix.

Both earlier gaps stay closed:

  • an unposted card gets one no-options pointer;
  • the fallback keeps the destination and link, and rewrites the record.

48/48, 26/26 and 48 neighbouring suites pass, and CI is green. Merge after #5946.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current exact head 9188fceb580b68d358e88ed5ffceaf2eff23a866, directly stacked on #5946 at a7e6bd411e35605601df5d292faacfba1e876ce5.

Verdict: CHANGES_REQUESTED — not ready to merge. The ordinary #1/#4 paths are substantially improved, but two failure boundaries still violate their stated durability contracts.

  1. Blocking — the “one unposted-card pointer per card” transition is neither crash-safe nor exclusive. send_card_pointers() checks state, publishes the proactive file, then stamps pointer_at. Killing the process with os._exit(17) after the real file publication but before the stamp left pointer_at=None; the next reminder produced a second distinct pointer file for the same card:

    ...pointer-1793085822.txt
    ...pointer-1793085824.txt
    

    Two synchronized reminder processes likewise both published, and a projector interleaving between the check and publication still queued a pointer after the card was posted. The sequential two-pass test cannot exercise these windows. Coordinate projection eligibility, a durable pointer claim, and publication through one recoverable writer; add process-crash, overlapping-process, and projector-interleave regressions.

  2. Blocking — the definite-failure fast path can delete the only local evidence before store history is committed. _redeliver_now() rewrites the landed row and deletes the re-held outbox entry, unlike Outbox.flush(), which commits history before deletion. With both marker writes failing and card storage failing definitely, the production-store probe reported:

    history_error=OSError: state marker read-only
    held_ids=[]
    store_history=False
    offline_open=0 offline_unavailable=False
    

    The row existed and carried the fallback record, yet a later room outage was misreported as a measured zero. Commit history before releasing the redeliver hold, or retain it for reconcile, and cover fallback + marker failure.

Positive evidence: the advertised suite passed 48/48; the four existing real-process crash cases passed 20/20 across five runs; live-card filtering (including --force), no-options pointer content, question-before-card ordering, row-returned fallback link, redeliver=true, and landed-but-unconfirmed row reconciliation all passed independent controls. The child-diff review scanner passed with a firing positive control, and current CI/CLA are green.

The PR body also states that no real post-restart round trip has been run. Because this changes the live card/delivery path, that witness remains an approval gate after the blockers above are fixed. Worst case today: duplicate or late owner pointers, or an unanswered question disappearing from local pending-question accounting during a room outage.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact current head baf7f062862e8eb2c63dccfad0ee6ee4efcc3e59, cumulatively over parent 689f359f0ac69f08fcb42c951fefae9603889251.

Verdict: CHANGES_REQUESTED — not ready to merge. The fallback/history loss is fixed, and the new locked claim fixes overlapping reminders publishing two pointers. The pointer transition remains non-recoverable, the cumulative parent has an owner-authority bug, and the live-path witness is still absent.

  1. Blocking — a failed pointer publication is now suppressed forever. claim_pointer() durably stamps pointer_at before send_card_pointers() publishes the file (ask.py#L182-L221). With write_proactive raising once:

    first=FAILED — OSError: results read-only
    pointer_at=<set>
    pointer_files=[]
    retry=[]
    

    The new failure/crash tests explicitly pin no retry. Worst case: the gateway never posts the card, one file write fails or the process dies after the claim, and every later reminder suppresses both the pointer and normal text, so the owner is never alerted. This is not the recoverable writer requested in the prior review; it trades duplicate publication for permanent loss. Keep a recoverable pending/published transition and cover failure-after-claim recovery.

    The projector interleave is also still open: production projection sends first and records its ledger afterward. If that send lands between pointer claim and file publication, the pointer still publishes after the card was delivered. The new test covers projection before claim, not this send→ledger window.

  2. Blocking cumulative interaction — owner Resolved can still produce an actionable click. The current parent returns actionable=True, stale=False for Status=Resolved beside Closed=Answered+claimtok@host when stale-mark clear() fails, because the matching token is accepted before terminal owner Status. Full repro and line evidence are in the current-head #5946 review. The child inherits that behavior.

  3. Approval gate — no real post-restart round trip is recorded. This newly activates the AG2 Space card/delivery path, and the PR body still says none was run. Per REVIEW.md lesson 15, unit/harness proof cannot substitute for one real card→owner click→task→ack flow through the restarted service.

Positive evidence: the prior fallback/history blocker is fixed under marker-write failure (held=1, offline waiting=1); overlapping pointer processes now produce one claimant/file; decision-card 52/52, card-click 28/28, room DB 51 passed with one optional skip, and the existing report-feedback/worker-pool regression suites pass. All 36 current check runs and CLA are green. Those results do not cover the permanent-loss and cumulative failure modes above.

@sonichi

sonichi commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

CI at 8c444dd (run 40468513295) had two red checks. I don't believe either is caused by this PR; I've re-run the failed jobs.

Shard 4/9 failed on one test, tests/skills/worker-pool/pool-restart.test.py:

FAIL: test_explicit_resume_then_restart_keeps_a_and_its_recorded_folder (__main__.RestartSessionContinuity...)
AssertionError: False is not true : pane pid 54004 survived SIGTERM for 60 s

"tsc + tests (clean install)" is the aggregate gate. Its log shows only python-standalone-tests FAILED (failure), so it is red because the shard failed.

Why I'm calling the shard failure a flake:

  • It fails on main too. The same test failed in the standalone shards of two main runs, 40445548879 and 40382391438. It also failed on other branches: run 40451375201 (sec/t5-chat-delivery-context-policy), run 40408270775 (sec/t3-agent-api-authz), and run 40438855058 (the earlier feat(pending-questions): ask-owner sends a decision card when --default or --option is given #5942 branch).
  • The diff doesn't touch that path. The test kills a real tmux pane process and waits for it to exit. This PR's only worker-pool change is how pool_wedge_cards.py builds its card objects. The test loads that file only indirectly, through pool_remedy, and never runs it. Nothing in the diff touches process spawning, signals or tmux.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact current head 8c444dd596f661cac94e3216fdac64b81570e088, child-only and cumulatively over exact parent 9794fd11c35bd3445625b82990807b3b4ee57a14.

Verdict: CHANGES_REQUESTED — not ready to merge. The requested recovery change works: an unpublished claim is retried after 300 seconds, the real-process crash/failure tests pass, and a fix-removed control fails those exact cases. Four child-layer issues and the live-path gate remain; the exact parent also remains blocked.

  1. Blocking — rollback to the direct parent restores a second text surface beside the live card. I created a card at this head, then loaded exact parent 9794fd11 against the same workspace. The card file remained live, but the old reminder did not recognize the child’s card record:

    child asked_recently=true
    parent asked_recently=false due_count=1 reminder_deliver_calls=1
    card_file_exists=true
    

    A normal rollback can therefore present the owner with the decision card and a competing text question, violating the advertised one-surface invariant. Add a downgrade-safe representation/reader transition and pin it with the direct-parent reader.

  2. Blocking — mark_pointer_sent() can revive an archived terminal card. It reloads a token-matching requirement and saves it without a terminal check. The store loads live and then archive, but save() always writes live. Exact production-code interleave:

    claim -> publish -> settle/project/archive -> mark_pointer_sent(old token)
    live=True archive=True live_terminal=True pointer_sent_at=<set>
    

    This violates the one-location store invariant; the same manager already guards this at link_blocked_task() because saving a terminal record “would revive an archived one.” Refuse the mark for terminal/archived records and add the claim→settle→archive→mark regression.

  3. Blocking — the five-minute claim is immediately stealable when wall time moves backward. The freshness condition requires claimed_at <= now. Exact probe:

    claim(now=T)   -> token A
    claim(now=T-1) -> token B
    A != B
    

    Two publishers can therefore proceed one apparent second apart. Treat a future claim timestamp as fresh, as the sibling click lease does, and add a rollback-clock regression.

  4. Blocking — the previously reported projection interleave is still open. After the locked claim, send_card_pointers() publishes without revalidating projection. Injecting record_projection(revision=1) after claim but before the real write produced:

    projection_revision=1 pointer_files=1 published=1
    

    The new test covers projection before claim only. Documenting the remaining window does not close the earlier requested synchronization/revalidation; either close it or obtain explicit maintainer acceptance of the false-positive pointer contract.

  5. Approval gate — no live post-restart round trip is recorded. This activates a live AG2 Space delivery path, and the current PR body still says none was run. Per REVIEW.md, approval needs a restarted card→owner click→task→ack witness; this review was not authorized to restart services.

Positive evidence: failed publication and death-after-claim now retry correctly at the boundary; overlapping reminders elect one claimant in ordinary time; the fallback/history fix and cumulative owner-Status ordering fix pass. Focused checks passed 402 tests with one optional pycrdt skip; child-only and cumulative review scans plus their positive control passed; current CI and CLA are green. git diff --check has only the existing extra blank line at EOF in src/hitl/cards.py.

@sonichi

sonichi commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

@keweichen, replying to your review at 8c444dd5: all four findings are fixed in place at 1864e4c, rebased onto #5946 at 6c57b3e. Each new test fails with its fix removed.

1. A rollback to the direct parent brings back a text surface.

2. mark_pointer_sent revives an archived card.

  • Fix: it now refuses a terminal record.
  • Test: your claim → settle → archive → mark interleave. The live file is not recreated and no pointer_sent_at is written. A cancelled, unarchived card is also left alone.

3. A back-stepped clock can steal a fresh claim.

  • Fix: a claim timestamp in the future counts as fresh until its window passes, the same rule the click lease uses.
  • Test: claim at T, then claim at T−1 and at T−5000 both return None. At T+300 the claim is taken.

4. The projection race after the claim.

  • Fix: just before writing the file, the card is re-checked under the HITL store lock (live, unposted, still this claim's), and the file is written inside that lock. A post recorded after the claim now cancels the pointer.
  • Test: your injection, record_projection(revision=1) after the claim. It now yields no pointer file and no sent mark.
  • Remaining window, bounded and documented: the projector records its ledger only after its send returns. So a send still in flight cannot be seen from here. That window is one projector send, and the pointer it can produce only says the card may not have arrived; it is not a decision surface. Closing it fully would need the projector to honour the pointer claim, a bridge change. If you want that rather than the bounded window, say so and I'll raise it with the coordinator.

The live post-restart round trip is still to come; Chi has OK'd it for after these land. Tests: decision-card 57/57, crash tests 3/3 repeated, and every affected suite passes. Changed-line coverage against the parent is 99.6%.

@sonichi
sonichi requested a review from keweichen October 27, 2026 09:14

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head eb2be77c4d46d1a1a46f881de4a20933b3a073bd on exact parent afe5424861c8d0e60141a84dbfa7e0e43d40c198. CHANGES_REQUESTED solely for the required live-path evidence before merge.

All seven code fixes now pass focused verification. In particular, the pre-publication ledger recheck closes the actionable post-claim race; rollback no longer emits competing text; terminal cards cannot be revived; and future-dated claims are not stolen. The remaining interval while a synchronous gateway card send is in flight is acceptable: at worst it publishes one extra non-actionable pointer saying the card may not have arrived and that replying does not answer it, then converges after ledger recording. I am not requesting a projector change for that bounded window; teaching generic core projection about the skill claim would cross the core/skill boundary and could delay the real card until claim expiry.

Blocking evidence still missing: a real post-restart round trip through the live delivery path—restart/service identified, real decision card, owner click, relayed hitl_click task, token-bound action/ack, and terminal card, with timestamps/events/logs. Harness-only proof cannot substitute for that witness under REVIEW.md.

Code evidence: current focused decision-card cases pass; four claim/publish/sent preservation cases pass; projection is 9/9; the full patch-equivalent decision-card suite was 57/57; the pointer-recheck mutant is caught; review checks pass with a positive control. Current CI also has two apparently unrelated timing/harness failures that pass on the exact parent and locally (WhatsApp guided-connect and Sparrow wake-loop timing); those still need a green rerun or triage before merge.

@sonichi

sonichi commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

CI at eb2be77 (run 40480318922) had three red checks. I don't believe any is caused by this PR. I've re-run the failed jobs and left the head unchanged.

  1. ag2-sparrow publish gate: packages/ag2-sparrow/tests/test_on_task_wake.py, test_failing_or_slow_command_never_blocks_or_loses_the_task, failed with AssertionError: unparseable: the pull loop waited 5.3s. It is a timing assertion on the gateway's pull loop.
    • The same failure appeared on another PR: run 40409456101 (sec/t1-phone-call-isolation).
    • It passes 3/3 locally on this head and on main.
  2. Shard 6/9: tests/whatsapp-guided-connect.test.py, events mode preserved when the build supports it, failed with ERROR: auth ended without a valid session. The test drives fake wacli shell stubs with sleep and --timeout windows, so it is sensitive to a loaded runner.
    • It passes 3/3 locally on this head and on main.
    • I found no other failing run for it among the last 200 CI runs, so its flakiness rests on the local passes plus there being no code path from this diff.
  3. "tsc + tests (clean install)": the aggregate gate. It is red only because the two above failed.

No code path from the diff. This PR changes only:

  • the pending-questions skill;
  • src/hitl/cards.py;
  • skills/report-feedback/report-feedback.py;
  • skills/worker-pool/scripts/pool_wedge_cards.py;
  • scripts/ask-owner.py (docstring);
  • docs and tests.

Nothing under packages/ag2-sparrow/ or the WhatsApp skill is touched, and neither failing test imports any changed file. The previous run on this branch failed only tests/recovery-issues.test.py (a missing telemetry cause name). That was a real defect, inherited from #5946 and fixed there in afe5424, which this head includes.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at eb2be77 (agent review).

Both blockers are fixed, and I reproduced each in real processes:

  • A process that dies after the pointer claim is retried after 5 minutes and delivers exactly once.
  • Six overlapping reminders queue a single pointer.

History is now committed before a fallback hold is released.

Tests: the rebase onto #5946 is clean. The suite passes 57/57, 48 neighbouring suites pass, and CI is green.

Nits:

  • The sent-mark token check is untested.
  • pending_questions_ask.py:163 holds a future-dated claim for the whole skew, the same issue as in #5946.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact child head d8707ffe on exact parent d9f49af5, both child-only and cumulatively. CHANGES_REQUESTED — not merge-ready.

Blocking — the pointer first-sight bound is not scoped to one claim generation. claim_pointer() has the same two adjacent failures as the parent lease logic:

  1. With retry window W=300, pointer_claim_at=T+299, and first observation at T, the call correctly refuses an immediate replacement and records pointer_skew_seen_at=T. It still refuses at T+300, however, and issues the replacement only at T+599: once the local clock catches the original stamp, lines 172–173 switch back to the original deadline and discard the first-sight bound.
  2. Line 175 retains the old pointer_skew_seen_at when it writes a new claim. Exact writer trace: observe far-future claim A at T; allow a delayed retry at T+600, producing claim B; then observe B at T+599, only one second behind its stamp. The writer immediately emits claim C (C != B) because the marker from A still says its window elapsed. The fresh retry is therefore replaced on first sight, which is exactly the no-first-sight-transition invariant this commit is meant to preserve.

Please keep the first-sight deadline effective for the whole current generation and clear/reset it whenever a new claim is issued. Add the W-1 near-future case and a delayed retry followed by a one-second rollback. The current test at lines 464–472 covers only one far-future generation.

The cumulative parent is also blocked by the corresponding lease-generation defect: #5946 (review)

Approval gate still open — live-path witness. The PR body still records no real post-restart round trip. Because this activates the AG2 Space card delivery path, approval still requires the previously requested restarted card → owner click → relayed hitl_click task → token-bound action/ack → terminal-card evidence under REVIEW.md lesson 15.

Evidence: exact-head decision-card suite 58/58; the independent focused run passed 493 unittest cases with one optional skip plus 28/28 outbox contract checks; child review scan and its positive control passed. GitHub reports the exact head open, mergeable, and currently clean. These green results do not cover the two generation-boundary traces above.

@sonichi

sonichi commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

@keweichen, replying to your review at d8707ffe: fixed at a5f59c4, rebased onto #5946 at 58b4de3.

The fix. The pointer claim now uses the same elapsed-time lease as the click (pending_questions_lease), with window POINTER_CLAIM_RETRY_SEC = 300 s and POINTER_MAX_STEP_SEC = 150 s. pointer_claim_at and pointer_skew_seen_at are gone. A claim already on a card is credited each reminder pass under the HITL store lock and kept until its lease is spent. A new claim gets a new token and a new lease, so nothing leaks across generations. The invariant, the trade-off and the restart behaviour are stated in the #5946 reply.

Your traces:

  1. A claim stamped W−1 ahead: never replaced on sight, and nothing is credited while the clock is behind the stamp (test_a_claim_stamped_just_under_a_window_ahead_is_never_replaced_on_sight).
  2. A delayed retry followed by a 1-second rollback: the fresh claim is kept, with no replacement.

Both are rows of test_the_pointer_claim_lease_table: one step; W−1; 1 s back; catch-up with no double credit; W gives a new claim with a new lease; 1 s back after the renewal keeps it; the renewed claim counts from its own start; exactly one replacement overall.

Kept from the last round: a crash after the claim, and a failed publish, are both retried once the lease is spent (real processes); overlapping reminders publish one pointer; a terminal or archived card is never revived; and a post recorded after the claim cancels the pointer.

decision-card 59/59, plus every affected suite, pass. Changed-line coverage against the parent is 99.6%. The live witness is still pending.

sonichi and others added 2 commits October 27, 2026 07:25
…ver-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>
… 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>

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact rebased head fcd54bc5d725687eaec30d8359b4aa6ba1ae0469 on current main 43b3c7c7894b6227722a3dcf31f872f0526c0ee7.

CHANGES_REQUESTED — the code layer remains clear; approval is still gated on the complete live round trip.

The rebase did not change the reviewed patch: old parent 6cf31ae9 and current main have the same tree, old head e6a001d0 and this head have the same tree, all eight commits map = in git range-diff, and both binary child diffs have SHA-256 09e26f2470965fa8cbd7247d171783f49ab50ac569a545df0027afdf170ceaa7. I found no new code blocker.

The supplied witness is useful but incomplete. It proves the owner button reached hitl_click, the click was token-claimed, and the local action/ack resolved the requirement. However the required terminal-card leg failed: the revision-3 EDIT was rejected with HTTP 400 at 13:51:14Z, kept retrying, and its ledger was later settled manually. Thus no terminal card landed, and the evidence does not identify a restarted build/SHA that completed the full trace. The PR body still says no live round trip has been run.

That 400 is a current-main wire defect which #5955 is intended to fix; #5955 itself now has requested changes and still owes its post-fix witness. Please fix and verify #5955 first, update/rebase this branch as needed, then record the restarted real-service trace: card creation/delivery → owner click → relayed hitl_click → token-bound action/ack → successfully projected terminal card. I did not restart the owner's service.

Exact-head evidence remains positive: decision-card 60/60, card-click 50/50, room DB 51/51 (one optional skip), affected HITL suites 141/141, the production grace/lease regression fails on old 62a442f7 with [0,0,0] != [0,150,300], git diff --check and review scanner are clean, and current CI plus CLA are green. The latest prior approval and my prior review were attached to old e6a001d0, not this head.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at fcd54bc (agent review).

The rebase changed nothing: the diff against main (43b3c7c7..fcd54bc5) is byte-identical to the reviewed 6cf31ae9..e6a001d0 (1,885 lines, index lines included).

Tests at this head: 60/60 decision-card, 50/50 clicks and 47 neighbouring suites pass.

Some CI checks were still running when I posted.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes at fcd54bc, for one reason: two of the new or changed assertions test the module's source text, not its behavior.

The rest of the suite is strong. Locally at this head, decision-card 60, room-db 51 (1 skip), pool-wedge-cards 26 and report-feedback 95 are all OK. The crash tests with a real os._exit are a good way to pin the ordering. CI and CLA are green, and there are no unresolved threads.

Asks

  1. tests/pending-questions-decision-card.test.py, test_the_pool_store_handle_is_the_helpers: this reads pool_wedge_cards.py with read_text() and asserts "from hitl.cards import manager_for" and src.count("choice_request(") == 4. A refactor that keeps the behavior turns it red, and a regression that keeps those strings stays green. Please make it a behavior test, like the test_report_feedback_asks_through_the_helper test beside it: patch cards.choice_request (with wraps=), drive each wedge card builder, and assert on the calls and the stored requirement.
  2. tests/pending-questions-room-db.test.py, the new core.index("Outbox(ws).save(") < core.index("def create_card(") pin: this compares a call site (line 475) with the position of a function definition (line 514). That says nothing about runtime order, because create_card is called from elsewhere (line 570). The decision-card crash tests ("death while holding the question: no card exists") already pin hold-then-store as behavior, so please drop this assertion or replace it with a reference to those tests.

Also

  • The live round trip is still owed. keweichen's witness shows the terminal EDIT failing with HTTP 400. That is the wire defect #5955 fixes, so this should merge after #5955, and the round trip should be run on a build that has both.
  • No hardcoded workspace paths in the added lines.

— Sutando (susans-MacBook-Pro)

… 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>
@sonichi

sonichi commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review fix at 02cae90: the stored-card pointer now runs unattended.

Confirmed: the 30-minute pointer for a stored-but-unposted card only ran under --notify, which is on demand. An unattended host therefore never sent it. The pointer completes the question's single ask rather than reminding, so it now runs on every pass of src/check-pending-questions.py. That includes the proactive loop's flagless pass. It is skipped in presenter mode, and --force overrides that skip. Text reminders still need --notify.

Claim, lease and retry are unchanged; they are now credited every pass instead of only when someone asks. The docs are corrected to match (CLAUDE.md/AGENTS.md "Pending decisions", the pending-questions and proactive-loop SKILL.md files, the rationale doc). That includes the crash-before-hold path, whose text goes out on the next --notify run, not "never silent".

New regression tests:

  • A flagless pass over a real card past its grace sends exactly one pointer and no reminder. This test fails when the fix is reverted.
  • Presenter mode holds the pointer for a later pass.

pending-questions-decision-card 62 OK, and the other pending-questions suites are OK.

sonichi and others added 2 commits October 27, 2026 12:22
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r tests

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at d5f3f60 (agent review). Some CI checks were still running when I posted.

Pointer behaviour:

  • The pointer now runs on every flagless pass, and presenter mode holds it.
  • Each change fails its test when reverted.
  • One pointer per card still holds under a crash and with six overlapping reminders.

Replaced source-text checks: they are covered by behaviour tests now. Each of these fails a new test:

  • bypassing choice_request;
  • storing the card outside the workspace store;
  • writing a row without a held record.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at d5f3f60. Both asks from my change request at fcd54bc are fixed.

My two asks

  1. The source-count test test_the_pool_store_handle_is_the_helpers is gone. pool-wedge-cards.test.py WedgeCardsUseTheHelper replaces it as a behavior test: it drives all five wedge card paths through raise_card, wraps cards.choice_request, and checks that each card is built once by the helper and stored through manager_for(ws).
  2. The Outbox(ws).save( vs def create_card( position pin is dropped. The new test_no_row_is_written_without_a_held_record makes Outbox.save fail and checks that no row is written while the owner is still asked. That is the order the pin was meant to protect, now tested as behavior.

New since fcd54bc: the one-time pointer for a decision card the gateway never posted now goes out on every flagless pass (not only --notify), and presenter mode holds it. Text reminders still need --notify. The docs say the same. When I limit the pointer to --notify again, test_the_unattended_pass_points_to_a_card_that_never_posted fails.

Tests at this head, run locally: decision-card 61, room-db 52 (1 skip), pool-wedge-cards 27, all OK. CI is green, license/cla is success, and there are no unresolved threads. No hardcoded workspace paths in the added lines.

Still holds (named, not blocking):

  • Merge after #5955. keweichen's witness showed the terminal EDIT refused with HTTP 400, which is the defect #5955 fixes, and #5955 is still open.
  • The live round trip is still owed on a build with both PRs (card created, owner click, hitl_click relayed, token-bound ack, terminal card edited). keweichen's open change request asks for the same thing.

— Sutando (susans-MacBook-Pro)

@sonichi
sonichi dismissed stale reviews from keweichen, keweichen, keweichen, keweichen, keweichen, keweichen, keweichen, keweichen, and keweichen October 28, 2026 01:31

Dismissed on the owner's decision: superseded by head d5f3f60, which qingyun-wu and liususan091219 approved with CI green. Re-review is still welcome as a follow-up.

@sonichi
sonichi merged commit e50b019 into main Oct 28, 2026
38 checks passed
@sonichi
sonichi deleted the feat/pq-card-ask branch October 28, 2026 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants