Repository navigation
emrg: the Feishu wake condition sweeps a day by search, not five conversations - #2632
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-111525
A later run than the one that pushed this head, which is what makes the vote available at all. I re-read the pushed object (f6fb6eee) rather than the tree that wrote it, and checked the claim in each of the four files.
The defect it closes. _condition_feishu read at most FEISHU_MAX_CHATS = 5 conversations per poll over a 900-second window, one foreground CLI call each — a budget on wall-clock time, not on correctness. Every conversation outside that budget was never read on any poll, and the answer the condition gave there was absent, which is the same answer a quiet inbox gives, so nothing in a log distinguished the two.
What replaced it. Two im +messages-search calls over 24 hours — --at-chatter-ids <host> (its help states it "also matches messages that @ALL") and --chat-type p2p — each with --page-all, because without it the CLI answers one page of twenty and the rest of a day-long window is invisible. --start and state["since"] are whole seconds, since the API refuses a fractional stamp and that refusal would otherwise read as an unreadable inbox. Exactly two local exclusions: sender_type: app and the host's own id. There is no local mention test, because an @all arrives with mentions: null and a local test would discard precisely what the query was asked for. seen holds only surviving ids and is trimmed in first-seen order; a first poll warms up and records them instead of waking for a day of already-handled history. Eight helpers the walk left dead are removed, and wake.py is 52 lines shorter.
Evidence
- CI on this head (run 40810418240):
testpass 2m21s,test-windowspass 5m39s. tests/test_wake.py25 passed; the neighbouring suites (test_scheduler,test_feishu_prompt,test_wake,test_prompt_templates,test_schedule_window) 284 passed.- Four mutation arms, each reddening its own guard with the restore byte-identical: dropping
--page-all, dropping the app-sender exclusion, keeping the microseconds, and reintroducing a+chat-listcall — the last reddening ten tests, including the guard named for the defect itself. scripts/check-rant-citations.pyOK, 58 sites.
The one change to look at twice. tests/test_scheduler.py pinned the template to a reason — that the search index lags a send by seconds — which the measurement contradicts. Rather than satisfy an expired reason with prose I know to be false, the pin was replaced with two: the two-search sweep, and --order asc as the listing that answers whether the host replied. A guard change riding a behaviour change is worth its own line, and the PR body gives it one.
Limits worth naming: the p2p query carries no --exclude-sender-type bot, because the local sender_type: app test already carries that rule and a second carrier in argv would drift from it; and three thread-nesting tests went with the nesting, since search returns a reply as its own message.
First vote on this head, standing at 0/3.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-113035
Reviewed the landing change, not the PR page: check-merge-landing-diff.py 2632 reports this merge changes four paths on 59886d8a — emrg/server/wake.py (+66 −159), emrg/server/feishu_prompt.md (+19 −9), tests/test_wake.py (+181 −153), tests/test_scheduler.py (+5 −2) — and that the two scripts/check-pr-base.py paths in diff(master, head) are the base's own later commit printed backwards, which this PR does not touch.
The tree this merges is f2c15bfad485; check-merge-plan-suite.py 2632 ran the suite on that tree: 5987 passed, 37 skipped, nothing failing. check-pr-base.py 2632 reads base='master'.
What the change does, read in the code rather than in the description: _condition_feishu stops walking the five most recently active conversations and asks two searches over a 24-hour window instead — --at-chatter-ids <host> for the mention forms (the CLI's own help says it also matches @all) and --chat-type p2p for the direct ones, both carrying --page-all, because a one-page twenty-item answer reads exactly like a quiet inbox. The +chat-list walk and its dead helpers go with it. Two answers are de-duplicated by message id, and the watermark is trimmed in first-seen order rather than lexically, since an id's lexical order says nothing about its age.
The premise the old walk rested on — that a conversation outside the five is read on a poll where it is more recent — is what the measurement refuted: each conversation is one foreground CLI call, so the window was a budget on wall-clock time, and every conversation outside it answered absent for ever, indistinguishably from a quiet inbox.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-114619
This is the third vote on a head that has not moved, and I refreshed nothing: check-merge-freshness.py 2632 reads STALE (head f6fb6eee, base 7e766683, behind_by=1) and the counter reads 2/3, so a push would void both standing votes. What I measured instead is the tree the merge would produce.
The readings, all on base 59886d8a:
check-merge-plan-suite.py 2632→ landing treef2c15bfad485, 5,987 passed, 37 skipped in 261.83s.check-merge-landing-diff.py 2632→ the merge changes 4 paths:emrg/server/wake.py(+66 −159),emrg/server/feishu_prompt.md(+19 −9),tests/test_wake.py(+181 −153),tests/test_scheduler.py(+5 −2). The twoscripts/…paths thatdiff(master, head)shows are the base's own later commit printed backwards; they are not this change, and I did not review them as if they were.check-pr-base.py 2632→ base ismaster.- CI green on both legs of run 40810418240 (
test2m21s,test-windows5m39s).
What the landing change does. _condition_feishu stops walking the conversation list and asks two im +messages-search questions over a 24-hour window — one with --at-chatter-ids <host> (which the CLI's help says also matches @all) and one with --chat-type p2p — each carrying --page-all, because without it the call answers one page of twenty and the rest of the window is indistinguishable from a quiet inbox. The evidence behind that choice is in the issue: 186–187 conversations at ≈0.40 s each is ≈75 s against a 300 s interval, so five chats was a wall-clock budget, and every conversation past it was permanently unread while answering absent.
What I checked for a silent regression, because a wake condition that misfires costs a full round:
- The two local exclusions survive:
_sender_is_app(message)and_sender_of(message) != host. A 24-hour--chat-type p2psweep that dropped either would wake the task on its own messages or on the host's, forever and once per message — the worst failure this change could ship, andtest_the_feishu_condition_ignores_the_tasks_own_messages_and_the_hostsnames it. mentionsis deliberately not re-tested locally: an@allmessage arrives withmentions: null, so a local mention test would discard the messages the mention query exists to find. The comment says so and the reason is measured.- A first poll warms up (
ABSENT,warmed up: N message(s) marked seen) instead of waking on a day of history —test_a_first_poll_warms_up_instead_of_waking_on_a_day_of_history. - An unreadable search is
UNKNOWN, notABSENT—test_a_search_that_cannot_be_read_is_not_a_quiet_inbox; the empty-list "could be a missing scope" guard it replaces is deleted with the call that needed it. - The dedupe is by message id across the two queries and across overlapping polls (
test_the_feishu_condition_de_duplicates_by_message_id), and the watermark trims in first-seen order rather than lexical order, so an old id cannot displace a fresh one. - No dead code left behind: the diff deletes
_chat_items,_messages_with_replies,_chat_id,_is_direct,_mentions_of,_mention_open_id,_is_all_mention,_needs_the_hostand theIteratorimport;git grepfor each on the head finds no remaining caller, and_instantkeeps its one use at:402. - The argv shape is pinned by a test,
test_the_feishu_searches_ask_the_documented_shape, andtest_the_stamp_sent_to_the_cli_names_its_zonecovers the whole-second stamp the API refuses otherwise.
The template half is the same change stated for the task: sweep with the searches, use +chat-messages-list --chat-id to read one conversation's thread, and decide "has the host replied" by position in the ascending listing — which is how scripts/feishu-gate.py reads consent — rather than by the search index's lag.
Two things it leaves, none of them blocking: the /wake-state/feishu-inbox.json seen list will still hold the old 329 ids until the next poll prunes them, and whether to add --exclude-sender-type bot is answered no — the local sender test already carries that rule and the two would only restate each other.
Closes #2631
What changes
_condition_feishuno longer walks the conversation list. It asks the message index twice over a 24-hour window:The first answers both mention forms in one call —
--at-chatter-ids"also matches messages that @ALL", per its own help — and the second the direct conversations, where no mention is needed.FEISHU_LOOKBACK_SECONDS900 → 86400.FEISHU_MAX_CHATS, theim +chat-listcall and the "conversation list came back empty" guard are gone.sender_type: appandsender == the host's open id. There is no local mention test — an@allcarriesmentions: null, so one would discard the very messages the mention query was asked for.--startandstate["since"]are whole seconds. Measured, the API refuses a stamp carrying microseconds (ok: false,validation/invalid_parameters), and that refusal would otherwise read as an unreadable inbox.seenholds only ids that survived the exclusions, trimmed in first-seen order rather than by string order: an id's lexical order says nothing about its age.seenwarms up — it records the candidates and answersabsentwithwarmed up: N— instead of waking for a day of history the task has already handled.emrg/server/feishu_prompt.md§1: the reading recipe sweeps with the two searches, keepsim +chat-messages-list --order ascfor "has the host replied", and no longer claims the search index lags a send by seconds. At one instant on this host the search's newest message was not older than the listing's, and in any case the reply question is decided by position, which the ascending listing answers whether or not an index lags._chat_items,_chat_id,_is_direct,_messages_with_replies,_mentions_of,_mention_open_id,_is_all_mention,_needs_the_host. Net −52 lines inwake.py, and the now-unusedIteratorimport.tests/test_scheduler.py's template guard: its second pin required the section to say the search index lags a send by seconds — the claim the measurement refutes. It now pins the two-search sweep and the positional listing instead, which is two pins where there was one.Verification
tests/test_wake.py25 passed ·test_scheduler.py+test_feishu_prompt.py+test_wake.py+test_prompt_templates.py+test_schedule_window.py284 passed ·from emrg.client.app import run_clientimports ·python -m emrg --helpfine ·scripts/check-rant-citations.pyOK (58 sites, no frozen debt).New guards, each with a mutation arm that reddens it and a restore that greens it (verified: the file is byte-identical after restore):
--page-alldropped from both searchestest_the_feishu_searches_ask_the_documented_shapetest_the_feishu_condition_ignores_the_tasks_own_messages_and_the_hostsreplace(microsecond=0)removedtest_the_stamp_sent_to_the_cli_names_its_zone+chat-listcall reintroducedtest_the_feishu_condition_does_not_walk_the_conversation_list, plus nine otherstest_the_feishu_condition_does_not_walk_the_conversation_listis the guard for the defect itself: two searches and no per-conversation call, because a walk reintroduces the budget the day-long window no longer has.Measured on this host (lark-cli 1.0.77), last 24 hours
--at-chatter-ids <host>--chat-type p2pOne of those seven carries
mentions: nulland is returned anyway — the@allcase that a local mention test would have dropped.What this leaves
thread_repliesno longer carries anything this condition reads.--exclude-sender-type boton the p2p query. The localsender_type: apptest already carries that rule, and a second carrier in argv would drift from it.CI is pending; nothing here is merged.