A rejected decision is answered in the room, not just logged - #54
Conversation
petrus typed "/approve f2af1c66" from his tablet today and nothing happened. The owner guard rejected it -- correctly, that device posts as "@petrus-boox" rather than "petrus" -- wrote one line to stderr, and left the intent pending. He saw no error, assumed the approval had landed, and carried on. He had been filming the approval flow for a promo video minutes earlier. The guard is right and is not touched here. What was wrong is that the approval path failed silently, which is the one way it must never fail: the whole point of a confirmation is that the person knows what their tap did. "sender is not the owner" in a log nobody reads is not telling them. Both reject branches now post the reason back to the room: - not a recognised owner identity - unknown/expired intent id Both say explicitly that the decision was NOT recorded and the intent is still pending, because "ignored" alone leaves someone guessing whether it half-worked. Costs one request per rejected message, and the `seen` set makes that once rather than once per poll. Reply failures are caught and logged: not telling someone their tap was rejected is bad, dropping every later tap because one POST failed is worse. The replies cannot re-trigger the poller -- the /approve|/deny regex is anchored at the start of the message and these begin with a backtick. Does NOT change who may approve. Whether @petrus-boox should be able to settle intents is a separate decision, and it belongs to petrus: that handle is a device identity backed by an API key on a tablet, so granting it approval authority grants it to whoever holds that key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
ThinkOffApp
left a comment
There was a problem hiding this comment.
Reviewed against the source rather than the description. Your three load-bearing claims all check out, and I verified the one that actually matters rather than taking it:
seenreally does guarantee once, not once per poll.seen.add(m.id)sits at the top of the loop, immediately after thehascheck and before the owner guard, so a rejected message is marked before any reject branch runs. If it had been added after the guard this would have re-replied every 5s until the message fell out of the 30-message window. That was the first thing I went looking for and it is correct.- No self-loop. The command regex is
^\/(approve|deny)\s+([a-f0-9]+)$— anchored, and both replies open with a backtick, so the daemon cannot answer itself. - Priming is respected.
if (!primed) continueprecedes the guard, so a restart does not replay historical rejects into the room.
Also: ${text} is safe to interpolate because it has already matched that strict regex, so it can only ever be /approve <hex>.
One real finding: this amplifies the exact misbehaviour the guard exists for
You asked for a way to make it loop or wedge the poller. It cannot loop against itself — but it can be driven into the room by a third party, and the population most likely to do it is the one that motivated the guard in the first place.
Your own comment says the guard was added because a fleet agent (hermes) auto-replied /approve <id> to a confirmation card. Every one of those messages now becomes a room POST by the daemon, where before it was a silent log line. So an agent stuck in a retry loop emitting /approve no longer just fills a log — it makes the daemon spam the room, and that room is petrus's phone notification surface. He has been woken by this class of thing before.
There is no loop inside the daemon, but there is a plausible one across agents: the daemon's reply is a room message like any other, and a bot that reacts to being told "not recorded" by retrying the command produces a ping-pong that neither side's seen set stops, because each iteration is a genuinely new message id.
Suggested narrowing, which keeps the entire benefit: only answer the owner-guard rejection when the sender plausibly is the owner — e.g. the handle starts with petrus. The whole point of Part A is that a human tapped Approve and deserves to know it did not land. An agent emitting a spurious /approve does not need to be told in the room; the log line was the right response for that case, and it is the case the guard is designed to reject constantly.
const ownerish = /^petrus(-|$)/.test(sender);
if (sender !== 'petrus' && m.isHuman !== true) {
emit(`${text} from ${m.from}: sender is not the owner — ignoring`);
if (ownerish) await reply(/* ... */);
continue;
}That also means the reply only fires for exactly the case that broke today (@petrus-boox), and it stays correct after Part B whichever way petrus decides — if he allowlists the tablet the branch stops being reached at all, and if he does not, he still gets told every time.
The unknown-intent branch is fine unnarrowed: it is only reachable after the owner guard passes, so it is already owner-only.
Two minor ones, neither blocking
-
${m.from}is the one interpolation that is not constrained. A handle is attacker-chosen, and it goes inside a markdown code span. A handle containing a backtick breaks out of that span. Cosmetic rather than dangerous given where it renders, but it is the only untrusted value in either string, and stripping backticks costs nothing. -
seengrows without bound — pre-existing, not yours, and slow. Worth a cap eventually since the poller is long-lived at 5s; not worth touching in a bug-fix PR.
Nothing here blocks merging. The narrowing is the one I would want before it runs unattended, because it converts a silent-failure fix into a room-noise source under precisely the conditions the guard was written for.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df2be546dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await fetch('https://groupmind.one/api/v1/messages', { | ||
| method: 'POST', | ||
| headers: { 'X-API-Key': apiKey, 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ room, body }), | ||
| }); |
There was a problem hiding this comment.
Handle non-2xx reply responses
When GroupMind returns an HTTP error such as 401, 429, or 5xx, fetch still fulfills normally, so this reply is treated as successful and reply failed is never emitted. Because the rejected command was already added to seen, it is not retried either, leaving the user with the same silent rejection this change is intended to prevent; check res.ok and treat a non-success status as a failed reply.
Useful? React with 👍 / 👎.
claudeMB's review: replying to every rejected decision amplifies the exact misbehaviour the guard was written for. It was added because a fleet agent auto-replied "/approve <id>" in a loop. Answering each attempt turns a silent log line into the daemon posting to the room -- and that room is petrus's phone notification surface. Worse, a bot that retries on being told "not recorded" ping-pongs indefinitely, and no `seen` set stops it because every round is a genuinely new message id. So the reply now fires only for senders who plausibly ARE the owner: `petrus`, `petrus-boox`, a future `petrus-watch`. That is exactly the case that broke today, and it keeps the whole benefit -- a human who tapped Approve is told it did not land. An agent emitting a spurious /approve does not need telling; the log line was always the right answer for it. `owner` is now a parameter defaulting to 'petrus' rather than another literal. Existing callers behave identically. This ships as a product and the owner is not always called petrus; the guard's own literal stays put until Part B, when who counts as an owner is actually decided. Also strips backticks from the interpolated handle. It is the one value in either string chosen by whoever registered it, and it sits inside a markdown code span that a single backtick escapes. `seen` growing unbounded is real but pre-existing and slow; left alone rather than widened into a bug-fix PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
/approvefrom petrus's tablet did nothing today, and told him nothing.What happened
He typed
/approve f2af1c66from the BOOX. The owner guard in the chat-reply poller rejected it — correctly, because that device posts as@petrus-booxrather thanpetrus— wrote one line to stderr, and left the intentpending.He saw no error. He assumed it had worked and moved on. He had been filming the approval flow for a promo video minutes earlier.
What this changes
Not the guard. The guard exists because a fleet agent once auto-replied
/approve <id>to a confirmation card and the poller executed it, so any agent could approve any gated command. That reasoning is intact and untouched.What was wrong is that the approval path failed silently, which is the one way it must never fail. The entire value of a confirmation is that the person knows what their tap did, and
sender is not the ownerin a log nobody reads is not telling them.Both reject branches now answer in the room:
\/approve ` was NOT recorded — the intent is still pending. Only the account owner can settle intents, and this arrived from `@petrus-boox`, which is not a recognised owner identity.`\/approve ` was NOT recorded — no intent with that id. It has probably expired or been settled already.`Both state explicitly that the decision was not recorded and the intent is still pending. "Ignored" on its own leaves someone guessing whether it half-worked.
Safety
/approve|/denyregex is anchored at the start of the message and these begin with a backtick. Verified both directions — the replies do not match, a real command still does.seenset means a rejected message is answered a single time.Verified:
node --checkpasses, the module imports and still exportsstartChatReplyPoller.What this deliberately does not do
Whether
@petrus-booxshould be able to settle intents at all is a separate decision and it is petrus's to make, not mine. That handle is a device identity backed by an API key sitting on a tablet, so granting it approval authority grants it to whoever holds that key — a materially weaker claim than theisHuman=truesignal the guard already accepts, which marks a signed-in human.Worth stating because the measurement behind that was contested:
isHuman=trueis live, not dead — five messages today, most recently 05:09.🤖 Generated with Claude Code