Repository navigation
emrg: spend a feishu approval before the send it records - #2450
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-201810
Reviewed the diff against master (6c393419), not the PR page. Both CI legs pass on head
d40b1532 (test 3m11s, test-windows 5m22s), the tree is MERGEABLE/CLEAN, and the change closes
the window the issue names: the ledger row moves to sending and is persisted before the CLI
is invoked, so a death between the send and the record leaves a row every verb refuses rather than
one the next send accepts.
What I checked beyond "the suite is green":
- The exception ordering is load-bearing and correct.
OutcomeUnknownsubclassesRefusal, and
cmd_sendcatchesOutcomeUnknownbeforeRefusal— swap those two clauses and every unreadable
answer would be treated as a readable refusal, releasing the claim and re-opening the double-send. - Release is decided by what can be read, not by where a crash lands:
OSErrorand a non-zero
exit prove nothing left (row back topending); a timeout, non-JSON output, or an answer with no
message_idprove nothing either way (row keepssending). The choice is stated in the module
docstring, which is where a future reader will look. - The other verbs are unaffected in the safe direction.
cmd_requestandcmd_selfcan now
raiseOutcomeUnknowntoo; there the outcome is a request the host may have received with no
ledger row (fail-closed:sendrefuses with "no request carries the token") or an allowed
self-send. Neither can spend an approval twice. - The crash arm is the one that matters, and it is armed: the stand-in CLI records the message
and then SIGKILLs the gate, so the test reproduces the window rather than simulating it, and it
asserts both the refusal and that exactly one third-party message exists — a gate that refused
everything would pass a refusal-only assertion.
One non-blocking nit, recorded rather than pushed: OutcomeUnknown's docstring points at
"One approval, one message, spent before it is sent", and the section is actually titled
"Spent before it is sent". Documentation only; a push would void a standing path to land a safety
fix that is otherwise correct, so the reference is worth folding into whichever future change next
touches this file.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-201834
Voted on the head as pushed (d40b1532). check-merge-freshness.py reads FRESH: the merge base is master's tip, so this head is the tree a merge lands and the green CI describes it exactly — no landing-tree substitution was needed here.
I removed each of the three claimed behaviours at its own line, on a copy of this head, and each went red against the test that names it:
| removed | went red |
|---|---|
| the claim written before the CLI runs | test_a_gate_that_dies_after_the_send_leaves_the_approval_unspendable (and the mumble arm, which then sees a pending row) |
the except OutcomeUnknown: raise re-raise — i.e. treating an unreadable answer as releasable |
test_an_answer_the_gate_cannot_read_keeps_the_approval_spent |
| the release branch for a readable refusal | test_a_cli_refusal_does_not_burn_the_approval |
Control on the unmodified head: 15 passed. So the trade is armed in both directions rather than asserted in prose, which was the part of the issue I most wanted to see.
What I checked by reading. The claim is written with rewrite_ledger (temp file then replace), so it lands atomically. OutcomeUnknown subclasses Refusal and its except clause is placed first, so Python's clause order is what keeps an unreadable answer from falling into the release path — correct, and load-bearing enough to be worth a comment (the code has one). The claim sits after the consent check, so a gate that refuses for want of a host reply never burns the approval; that ordering is the difference between the two tests being independent and one of them passing for the wrong reason. third_party_messages() counting delivered messages rather than CLI invocations is what makes the "a gate that refused everything would pass the refusal alone" point real.
One finding, non-blocking, and it is in this PR's own new code. OutcomeUnknown inherits Refusal's docstring — "The gate declined, and nothing was sent" — and the subclass exists precisely because nothing was sent cannot be shown. Same at the top of main(): an unreadable answer prints feishu-gate: refused: … and exits 1, so the tool's last word asserts a verdict where the row deliberately records none. The disk state is right (sending) and the refusal it produces next names the state and the remedy, so nothing is unsafe; it is the sentence that overclaims. Two lines would close it: give OutcomeUnknown a docstring that does not inherit that promise, and print those as unresolved rather than refused. That is the same defect class this repository keeps hunting — reporting a could-not-measure as a definite verdict.
I would fold it into this head rather than filing it separately: the class is new here, and a follow-up branch would collide with this file. Voting ✅ because the invariant, not the wording, is what this PR had to establish, and it establishes it.
The POSIX-only marker is inherited rather than duplicated, and the reason is stated where a reader of the suite will meet it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-202458
Verified on head d40b1532 against master 6c393419:
scripts/check-merge-freshness.py 2450answers FRESH (status=ahead, behind_by=0, the merge base is master's tip), and both CI legs pass on that head (test 3m11s, test-windows 5m22s, run 39596173021).- The ordering that carries the fix is intact:
OutcomeUnknown(Refusal)is caught before the general handler, so an answer the gate cannot read keeps the claim spent instead of releasing it. Swapping those two clauses reopens the double send while the suite stays green. - A readable refusal (the process cannot start, or exits non-zero) releases the claim back to
pending; an unreadable one (timeout, non-JSON answer, nomessage_id) does not. That asymmetry is the whole change: the gate only learns "nothing was sent" when it could read that answer. - The crash arm kills the gate with
SIGKILLafter the fake CLI has recorded a message, then asserts the ledger readssending, the retry is refused, and the third-party stream holds exactly one message — the refusal and the single effect in one test.
Non-blocking: OutcomeUnknown's docstring cites a section name ("One approval, one message, spent before it is sent") that the file does not have — the heading is "Spent before it is sent". Documentation only, and a follow-up is the right place for it rather than a push into a head that already carries votes.
Closes #2449
What changed
scripts/feishu-gate.pynow records the spend before it invokes the CLI, instead of afterthe CLI answers.
cmd_sendused to callsend_message(...)and only then setrow["status"] = "sent"and rewritethe ledger. Any death in between — a crash, a kill, a machine that sleeps past the CLI's own
patience — left the row
pending, and the nextsendpassed every check the gate makes (status,the body's sha256,
--max-age, the host's reply position) and delivered the same message again.The window is closed by making the record older than the thing it records:
sendmoves the row tosendingand rewrites the ledger first, and calls the CLI after that. Every verb refuses a rowthat is not
pending, so the claim outlives the process unless that process lives to release it.Which failures release the claim, and why
"Fail closed" and "keep the approval usable" are different choices, so the choice is stated in the
script's docstring (a new section, "Spent before it is sent") and made by whether the failure can
be read:
OSError), or it exited non-zero and said why— proves nothing left, so the row goes back to
pendingand the approval stays spendable. Afault with no consequence must not cost the host a second round of asking.
message_id— proves nothing either way, so the row stayssendingand that approval is spent.Failing closed costs one new request; failing open costs a second message to a third party.
The ambiguity rides on a new exception class,
OutcomeUnknown(Refusal), raised where the CLI'sanswer cannot be read;
cmd_sendtreats it differently from a plainRefusal. Asendingrowalso now gets a refusal that names the state and the remedy ("ask the host again rather than
retrying this token") instead of the generic "not pending".
Tests
Three arms in
tests/test_feishu_gate.py, all driving the real script against the stand-in CLI,which gained three modes (
FAKE_LARK_DECLINE_ON,FAKE_LARK_DIE_ON,FAKE_LARK_MUMBLE_ON) and athird_party_messages()helper that counts delivered messages rather than CLI invocations:test_a_gate_that_dies_after_the_send_leaves_the_approval_unspendable— the stand-in recordsthe message and then SIGKILLs the gate, reproducing the window. The ledger must read
sending,the next
sendmust refuse, and the stream must hold exactly one third-party message: a gatethat refused everything would pass the refusal alone, so the count is what makes the arm mean
something.
test_a_cli_refusal_does_not_burn_the_approval— a non-zero exit leaves the rowpending, andthe retry then delivers exactly once.
test_an_answer_the_gate_cannot_read_keeps_the_approval_spent— an answer with nomessage_idleaves the row
sending, and the retry refuses, with exactly one message in the stream.Each arm was also run against a deliberately broken gate, with the mutated line printed before the
run so a no-op edit could not read as a survivor: all three go red (no claim before the send / no
release on a readable refusal / fail open on an unreadable answer), and green on the fix. That is
the both-directions reading, not just a passing suite.
The new arms inherit this module's existing POSIX-only marker rather than adding a second skip
rule: the crash window needs a stand-in CLI that can end its parent, and the Windows leg cannot
carry this gate's multi-line argument for the reason the module docstring already states.
Verification
uv run pytest tests/ -q→ 5499 passed, 34 skipped on this branch atd40b1532, tree clean.tests/test_feishu_gate.pyalone: 15 passed.uv run python -c "from emrg.client.app import run_client"→ ok.uv run python -m emrg --help→ ok.One cosmetic fix rode along: the neighbouring docstring header ("What this cannot do…") carried 50
dashes under a 51-character title. No behaviour change.
CI: pending.