fix(approvals): also deliver card to origin chat + verify clicker#2562
Closed
kky wants to merge 1 commit into
Closed
fix(approvals): also deliver card to origin chat + verify clicker#2562kky wants to merge 1 commit into
kky wants to merge 1 commit into
Conversation
Two coupled changes to make approval cards reliably reach the operator: 1. Belt-and-suspenders delivery: `requestApproval` now sends the card to the picked DM AND to the originating messaging group (the chat the agent was active in when it requested the approval). The DM stays the primary target — that's where privacy and routing dictate the card belongs — but a stale `user_dms` cache row pointing at an abandoned 1:1 the operator no longer watches can quietly route the card into the void. Posting to the origin chat too means the operator sees it where they're already paying attention. Skips the second send when origin and DM resolve to the same physical chat, and tolerates either delivery failing as long as at least one succeeds. Failure of all targets → notify the requesting agent. 2. Click authorization in `response-handler.ts`: now that the card can reach a wider audience (origin chat may have non-admin members visible), the response handler verifies the clicker's userId is in the approver list for the approval's session. Empty userId or non-approver click → log and ignore, leaving the approval pending for the actual admin. Without this, opening the delivery surface would also open the action surface; together they don't. Triggered by: install_packages approval for Homie's python3 install was filed and delivered (per logs) to the picked DM, but the DM was the operator's stale 1:1 they no longer watched, so they never saw the card. Approval sat unanswered until manual SQL approval. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Collaborator
|
@kky Appreciate the PR and understand the need but not usre this is a generalizable solution that belongs upstream. There are many cases where it would be incorrect to deliver to a group where it originated. Making it configurable in some way makes sense but for now I think this will need to stay in the realm of private fork changes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
.claude/skills/<name>/, no source changes)Description
What.
requestApprovalnow delivers the approval card to both the selected DM and the origin chat (the messaging group the agent was active in when it filed the approval).response-handler.tsverifies the clicker'suserIdis in the approver list before processing.Why. A DM can be stale; unused, unseen. To be sure it reaches the user, it is better to deliver approval to the chat that triggered the action in addition to the DM. This is the pattern with a channel like Signal, where a 1:1 "group" is used to DM different agents, all sharing the same Signal account.
Coupled with that: opening the delivery surface (origin chat may have non-admin members visible) without also tightening the action surface would let any visible member click the approve button. The clicker-verification half closes that loop.
How it works. Both targets are tried; partial failure tolerated (succeed if at least one delivery lands). If origin and DM resolve to the same physical chat, the second send is skipped. If all targets fail, the requesting agent is notified. The clicker check rejects empty
userIdor anyuserIdnot returned bypickApprover()for the approval's session — the approval stays pending.How it was tested. Verified against the original failure scenario: install_packages request from an agent now arrives in both the operator's primary DM and the originating group chat, and a non-admin member of the origin chat clicking the approve button is rejected with a log line while the approval remains pending.