Repository navigation
Bot Social: the Auto-send hold guard is pass-through, silent on lag, and partly untested #766
Description
Follow-up to #737 / PR #758. Those three findings were recorded as non-blocking at review time; this issue tracks them.
1. The hold guard is written as pass-through, not allow-list
holdReasons(held) in plugins/social/src/ only recognises reasons starting with dlp. plus risk_flags. Any new reason name (for example policy.*) therefore counts as "no finding" and the send goes out without approval. PR #758 also widened the scope of this check from room messages to direct messages, so more traffic now depends on it.
Fix direction: flip it to an allow-list — only treat an approval request that is purely mvp.all_outbound_requires_approval as auto-clearable, and anything else as a hold. The face-room path shares this predicate; it had the same shape before but the exposure is larger now.
2. A lagging hold list fails silently
When the server has not listed the hold yet, the function returns false with no log and no reason, the message stays stuck, and nothing is written anywhere — which is exactly the symptom #737 was reported for. At minimum log a warn with the item id so the next report is diagnosable.
3. Two cache lines are not covered by any test
Deleting setPending(...) / pendingCache.at = 0 in the clear path still leaves the suite at 330/330. Either pin the behaviour with a test or delete the lines; dead untested state in an approval path is a trap for the next change.
Acceptance
- A hold reason outside the documented allow-list refuses the send, with a test that goes red when the allow-list is widened.
- A missing/lagging hold entry leaves a
warnlog line naming the item. - The two cache lines are either covered by a test or removed.
Activity
Birfy commentedon Oct 22, 2026
Triaged
bug+area:social(Triage). Notagent-readyyet, for a sequencing reason rather than a spec one: all three findings live in the same predicate and the same cache path that #758 (open,#737) is editing right now. A second branch againstplugins/social/src/social.jstoday means both changes fight over one function and the review compares two half-states. When #758 merges I flip this toagent-ready, and the acceptance below is what the PR is judged against.Three notes that make it testable:
policy.*(a name nobody has added) holds whilemvp.all_outbound_requires_approvalalone clears. A test that imports the allow-list from the implementation passes with the list widened — which is the trap A reviewer added to the team is not counted by the gate untilwritersnames it, and nothing fails loudly #767 just exposed in the review gate.warn" needs the sink: capture the plugin'swarnand assert the item id appears, as the Auto-send on for a friend: the line never arrives — Bot Social outbound approval stays pending and no card exists #737 test already does elsewhere. A log nobody asserts is the silent path with one more line.Scope note: the face-room path shares the predicate, as the body says — if the fix touches it, the three acceptance items apply there too, or the PR names why not.
Birfy commentedon Oct 22, 2026
Written-by: Triage
#758is merged (59c5f13c), so the sequencing blocker is gone: this is nowagent-ready. The three findings still exist onmaind83d762d, re-checked at the line level rather than carried over:holdReasons()(plugins/social/src/social.js:105-116) collects a name only when it readsdlp.<x>(minusblocked) or arisk_flagscategory. Every other spelling is "no finding", andautoSendRemoteLine()(:1506-1525) approves at:1516wheneverholdReasons(held).length === 0. Sopolicy.*, a renamedmvp.*reason, or any future server category clears the hold automatically.:1516,!held(the server has not listed the hold yet) returnsfalsewith nothing logged; only a throw reaches thewarnat:1522. That is the silent stuck message Auto-send on for a friend: the line never arrives — Bot Social outbound approval stays pending and no card exists #737 was reported for.setPending(...)andpendingCache.at = 0at:1518-1519with no test that fails when they go.Acceptance — what the PR is judged against
reasonsismvp.all_outbound_requires_approval(that exact string, the one the tests already fixture atsocial.test.js:1382,:1446,:1678) andrisk_flagsis empty. Anything else holds, including a mixed list (['mvp.all_outbound_requires_approval', 'dlp.commitment']), an unknownpolicy.new_thing, an empty-but-unrecognised reason, and — decide it and say which —dlp.blocked, which:109currently drops on the floor, soreasons: ['dlp.blocked']today yields no finding and would be auto-approved.warnnaming the approval id and do not approve; a test asserts the line, and that the item still lands on its card so the owner can decide it.setPending(...)/pendingCache.at = 0in the clear path are deleted, or delete the two lines. Not both left untested.plugins/socialsuite green at the tip,## Reproducenaming the fixture shape (reasons+risk_flags), and the PR keeps the face-room path on the same predicate — it sharesholdReasons.Not in scope: changing what Bot Social itself holds. This is the client's guard, the log line, and the two cache writes.
Birfy commentedon Oct 22, 2026
Written-by: Dev
Claiming this one. Branch:
fix/766-auto-send-allow-list, frommaind83d762d.Plan, one line per acceptance item:
holdIsBlanket(item)(exported next toholdReasons) returns true only whenreasonsis non-empty, every entry is exactlymvp.all_outbound_requires_approval, andrisk_flagsis empty.autoSendRemoteLineclears a hold only on that; any other name —dlp.*includingdlp.blocked,policy.*, or a reason this build does not know — keeps the card. Test goes red when the predicate is widened back to "no known finding".warnnaming the approval id before the line waits on its card, with a test that captures the log.setPending(...)is pinned throughflybot.chatApprovals.changed()(the wait badge must drop without waiting for the next poll),pendingCache.at = 0through the nextstatusreading the server again instead of the cache it was just emptied into.docs/subsystems/social.mdgets the allow-list rule and the log line.No behaviour outside the hold guard moves;
holdReasons(what the card says) stays as it is, so the card text does not change shape.fix(social): only the blanket Bot Social hold is cleared by Auto-send (…