delete emoji approvals, make approval handling shared pkg#114
Merged
Conversation
Introduce a new ai-command package to centralize parsing of visible (`/...`) and hidden (`!ai ...`) commands with canonical names and aliases. Add an ApprovalCoordinator to pkg/ai-stream to normalize approval requests, parse/stamp approval commands, wait/resolve approvals, and publish lifecycle hooks; replace duplicated per-run approval logic with this coordinator. Integrate the registry and coordinator into the connector: use the command registry for parsing, use ApprovalCoordinator for request/resolve/publish flow, and simplify approval-related code paths. Update Matrix approval content rendering to use helper commands, and add/tests for command parsing and approval flows. Update README to reflect the new packages and behavior.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors approvals from reaction-based UX to a dedicated ChangesApproval Command Workflow Refactor
Stream Anchor Message Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/connector/stream_test.go (1)
118-126: ⚡ Quick winSynchronize the
Result()goroutine before asserting it is blocked.This negative assertion currently depends on the goroutine being scheduled within 50ms. If it hasn't entered
stream.Result()yet, the test can pass without actually proving thatResult()is blocked by anchor setup. Add a smallstartedchannel and wait for it before the timeout-based check.Suggested test hardening
resultDone := make(chan ai.Message, 1) + resultStarted := make(chan struct{}) go func() { + close(resultStarted) resultDone <- stream.Result() }() + <-resultStarted select { case result := <-resultDone: t.Fatalf("stream result completed before anchor setup: %#v", result) case <-time.After(50 * time.Millisecond): }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/stream_test.go` around lines 118 - 126, The test races on whether the goroutine has entered stream.Result() before the 50ms timeout; fix by adding a started channel and have the goroutine send a signal on started immediately after it begins (before calling stream.Result()), then in the test wait for started before performing the timeout-based read from resultDone to assert Result() is blocked; update the goroutine that writes to resultDone to also write to started, and add a receive on started in the main test flow prior to the select that checks for blocking.pkg/ai-stream/stream_test.go (1)
763-780: ⚡ Quick winLock in the removal of legacy approval aliases with a negative test.
This renamed test only proves the canonical command keys still work. Since this PR intentionally deletes emoji/reaction-style approvals, add a couple of assertions that legacy inputs are rejected so that behavior doesn’t quietly come back later.
Example addition
func TestApprovalResolverMatchesCommandChoices(t *testing.T) { choices := DefaultApprovalChoices() choice, ok := ResolveApprovalChoice(choices, "approve") @@ if !ok || response.Approved || response.Reason != "denied" { t.Fatalf("expected denial, got %#v ok=%v", choice, ok) } + if _, ok := ResolveApprovalChoice(choices, "✅"); ok { + t.Fatal("legacy emoji approval choice should not resolve") + } + if _, ok := ResolveApprovalChoice(choices, "yes"); ok { + t.Fatal("legacy alias approval choice should not resolve") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai-stream/stream_test.go` around lines 763 - 780, Update TestApprovalResolverMatchesCommandChoices to assert legacy/reaction-style approval inputs are rejected: after creating choices via DefaultApprovalChoices and existing positive checks, add negative assertions that ResolveApprovalChoice(choices, "<legacy>") returns ok == false for a couple of legacy strings (e.g. emoji or reaction aliases you removed), and do not call ApprovalResponseForChoice when ok is false; reference ResolveApprovalChoice, DefaultApprovalChoices, and ApprovalResponseForChoice to locate where to add these assertions so the test will fail if legacy aliases are ever reintroduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ai-stream/approval_command_test.go`:
- Around line 30-37: The test is timing-sensitive because
ApprovalRequest.ExpiresAt is set to the same 1s window as ctx's deadline and the
test blocks indefinitely on a bare `<-done`; to fix it, set ExpiresAt to a time
well past the context deadline (e.g., time.Now().Add(5 * time.Second)) so expiry
can't race the test, and replace the bare `<-done` with a select that waits for
either the done channel or a short test-specific timeout (using the existing ctx
or a new timeout) to fail the test instead of hanging; apply the same change to
the other instance of the test at the noted lines.
In `@pkg/ai-stream/approval_command.go`:
- Around line 57-67: ParseApprovalCommand currently validates choices against
DefaultApprovalChoices() which rejects request-specific choices; change it to
accept the pending request's choices (or remove validation here and let the
coordinator validate). Update ParseApprovalCommand signature to take the
request-specific choices (e.g., pass in the pending request's Choices) or return
a parsed approval payload without choice validation, then have the coordinator
call ResolveApprovalChoice with the request's Choices and perform validation
there; keep creation of the ApprovalResponse via ApprovalResponseForChoice and
stamping via StampApprovalResponse unchanged.
- Around line 101-127: The deferred c.Delete(request.ID) at the top leaves the
pending entry live while hooks run; move the deletion so the pending entry is
removed immediately after c.wait returns: remove the initial defer
c.Delete(request.ID), and call c.Delete(request.ID) right after response :=
c.wait(ctx, request, pending) (ensuring pending is cleared before executing
BeforeResponded/PublishResponded). Preserve deletion on early-exit/error paths
by adding short-lived defers or explicit c.Delete(request.ID) in error branches
where the function returns before the wait completes so we still clean up on
those paths.
In `@pkg/connector/approvals.go`:
- Around line 41-49: The current flow in PublishRequested (in
ApprovalCoordinator) queues the remote prompt via cl.UserLogin.QueueRemoteEvent
before calling publishApprovalInterrupt, which can leave an orphaned prompt if
publishApprovalInterrupt fails; to fix, make the side-effects atomic by calling
publishApprovalInterrupt(ctx, cl, publisher, portal.MXID, stream, request) first
and only on success call cl.UserLogin.QueueRemoteEvent(...) and then set
request.ApprovalEventID, or if you must keep the current order add a
compensating cleanup that cancels the queued prompt when
publishApprovalInterrupt returns an error (use the appropriate cancel/remove
method on cl.UserLogin with queued.EventID), ensuring request.ApprovalEventID is
only set after both operations succeed.
---
Nitpick comments:
In `@pkg/ai-stream/stream_test.go`:
- Around line 763-780: Update TestApprovalResolverMatchesCommandChoices to
assert legacy/reaction-style approval inputs are rejected: after creating
choices via DefaultApprovalChoices and existing positive checks, add negative
assertions that ResolveApprovalChoice(choices, "<legacy>") returns ok == false
for a couple of legacy strings (e.g. emoji or reaction aliases you removed), and
do not call ApprovalResponseForChoice when ok is false; reference
ResolveApprovalChoice, DefaultApprovalChoices, and ApprovalResponseForChoice to
locate where to add these assertions so the test will fail if legacy aliases are
ever reintroduced.
In `@pkg/connector/stream_test.go`:
- Around line 118-126: The test races on whether the goroutine has entered
stream.Result() before the 50ms timeout; fix by adding a started channel and
have the goroutine send a signal on started immediately after it begins (before
calling stream.Result()), then in the test wait for started before performing
the timeout-based read from resultDone to assert Result() is blocked; update the
goroutine that writes to resultDone to also write to started, and add a receive
on started in the main test flow prior to the select that checks for blocking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da12fae8-04f9-4cb5-a4a0-14a1e492ff2f
📒 Files selected for processing (14)
README.mdpkg/ai-command/command.gopkg/ai-command/command_test.gopkg/ai-stream/approval.gopkg/ai-stream/approval_command.gopkg/ai-stream/approval_command_test.gopkg/ai-stream/matrix/content.gopkg/ai-stream/stream_test.gopkg/connector/approvals.gopkg/connector/client.gopkg/connector/slash_commands.gopkg/connector/slash_commands_approvals.gopkg/connector/slash_commands_test.gopkg/connector/stream_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker
🔇 Additional comments (20)
README.md (7)
239-239: LGTM!
248-248: LGTM!
514-514: LGTM!
534-535: LGTM!
535-535: LGTM!
542-542: LGTM!
235-237: Confirm/approvearg options match the implementation
README lists/approve <approval-id> <approve|always|deny>, but the codebase search forApprovalChoice{ Key: ... }/ approval-relatedKey: "(approve|always|deny|always_approve)"returned no matches, so the choice-key mapping used by/approvestill needs to be checked in the implementation.pkg/ai-stream/matrix/content.go (1)
249-249: LGTM!pkg/connector/client.go (6)
103-103: LGTM!
119-119: LGTM!
1114-1114: LGTM!
2587-2603: LGTM!
2620-2623: LGTM!
2637-2643: LGTM!pkg/connector/approvals.go (1)
59-83: LGTM!Also applies to: 103-123
pkg/connector/slash_commands.go (1)
15-18: LGTM!Also applies to: 162-194, 207-250
pkg/connector/slash_commands_approvals.go (1)
14-22: LGTM!pkg/ai-stream/approval_command_test.go (1)
10-25: LGTM!Also applies to: 77-88
pkg/ai-stream/stream_test.go (1)
1049-1051: LGTM!pkg/connector/slash_commands_test.go (1)
59-60: LGTM!Also applies to: 75-75, 81-81, 87-87, 93-93, 134-145
Change ParseApprovalCommand to accept explicit ApprovalChoice lists and use those when resolving choices. Add ApprovalCoordinator.ResolveCommand to parse and resolve approval slash commands against pending requests. Adjust ApprovalCoordinator.Request to avoid double-delete of pending entries and ensure proper cleanup after wait. Update connector to call publishApprovalInterrupt earlier and use the new resolveApprovalCommand flow. Improve Client anchor handling: return queued EventID when available, fallback to deterministic event ID generation on timeout, and remove active streams when anchor confirmation fails. Update tests to reflect the new signatures and behaviors.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/connector/client.go (1)
2598-2613:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep waiting for anchor persistence even when the event ID is already known.
Line 2602 now returns as soon as
stream.eventIDis set, butfinalizeAssistant/failOpenAssistantuse this helper to ensure the anchor is actually editable. Knowing the MXID is not enough here:assistantFinalEditWithProjectiondrops the edit when the anchor row is still missing, so a fast completion can still lose the final/error edit if the store write lags behind the queued anchor.Suggested fix
func (cl *Client) waitForAssistantAnchorMessage(ctx context.Context, stream *assistantStreamState) error { if stream == nil { return fmt.Errorf("missing assistant stream") } - if stream.eventID != "" { - return nil - } if stream.portal == nil { return fmt.Errorf("missing assistant stream portal") } eventID, err := cl.waitForMessageEventID(ctx, stream.portal, stream.messageID, aiid.PartID("text"), 30*time.Second) if err != nil { return err } - stream.eventID = eventID + if stream.eventID == "" { + stream.eventID = eventID + } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/client.go` around lines 2598 - 2613, The early return in waitForAssistantAnchorMessage when stream.eventID is non-empty skips waiting for the anchor persistence and can cause dropped final/error edits; remove that early return and always call waitForMessageEventID (using stream.portal and stream.messageID with aiid.PartID("text") and the same timeout) so the function ensures the anchor row is editable even if eventID is already known; keep existing nil checks for stream and stream.portal and still set stream.eventID from the returned eventID if it was previously empty.pkg/ai-stream/approval_command.go (2)
108-115:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSynchronize access to
pending.request.
Requestupdatespending.requestafteradd()without holdingc.mu, whileResolveCommandreadspending.request.Choicesfrom another goroutine after releasing the mutex. A fast/approvecan race withPublishRequested, which is undefined under the race detector and can validate against stale or partially updated request state.🔒 Minimal locking fix
if hooks.PublishRequested != nil { var err error request, err = hooks.PublishRequested(ctx, request) if err != nil { return ToolApprovalResponse{}, err } - pending.request = request + c.mu.Lock() + pending.request = request + c.mu.Unlock() }- c.mu.Lock() - pending := c.pending[approvalID] - c.mu.Unlock() + c.mu.Lock() + pending := c.pending[approvalID] + var choices []ApprovalChoice + if pending != nil { + choices = append([]ApprovalChoice(nil), pending.request.Choices...) + } + c.mu.Unlock() if pending == nil { return ToolApprovalResponse{ID: approvalID}, false, nil } - response, err := ParseApprovalCommand(arg, pending.request.Choices, now) + response, err := ParseApprovalCommand(arg, choices, now)Also applies to: 147-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai-stream/approval_command.go` around lines 108 - 115, The update to pending.request in the PublishRequested hook is not synchronized and can race with ResolveCommand reading pending.request. Modify the code around hooks.PublishRequested (and the similar block at lines 147-153) to acquire the command-level mutex (c.mu) before writing pending.request (and keep it held while assigning the new request) or otherwise perform the assignment inside the same critical section that calls add()/sets up pending, ensuring pending.request updates and reads (e.g., in ResolveCommand) are protected by the same c.mu lock; update both the PublishRequested paths and any other places that mutate pending.request to use c.mu consistently.
164-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep lookup and delivery atomic in
Resolve.
Resolvedropsc.mubefore the channel send. Ifwait()times out or aborts andRequestdeletes the entry in that gap, this method can still send into the buffered channel and returntrueeven though nobody is waiting anymore. The caller will then reply with a success message for an approval that already expired.🛠️ Suggested fix
func (c *ApprovalCoordinator) Resolve(response ToolApprovalResponse) bool { if c == nil || response.ID == "" { return false } c.mu.Lock() - pending := c.pending[response.ID] - c.mu.Unlock() + defer c.mu.Unlock() + pending := c.pending[response.ID] if pending == nil { return false } select { case pending.response <- response: return true default: return false } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai-stream/approval_command.go` around lines 164 - 175, Resolve currently releases c.mu before sending to pending.response, allowing Request to delete the entry in between; keep the lookup and delivery atomic by holding c.mu while performing the non-blocking send: inside Resolve, lock c.mu, load pending := c.pending[response.ID], if pending == nil unlock and return false, then perform the non-blocking send to pending.response (select { case pending.response <- response: success = true default: success = false }), unlock c.mu and return success; refer to Resolve, c.mu, c.pending, pending.response and Request/wait interactions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ai-stream/approval_command.go`:
- Around line 141-145: The code currently treats a bare token like "/approve
approval-1" as missing the choice and sends it straight to ParseApprovalCommand
which expects a choice; change the branch in the approval parsing so that when
strings.Cut yields ok==false (i.e., only an approval ID was provided) you
synthesize a full command by appending the default choice token (use the
first/default entry from DefaultApprovalChoices()) to the approval ID and then
call ParseApprovalCommand with that synthesized string (keep the existing now
parameter and error handling). Reference symbols: strings.Cut usage around
approvalID, ParseApprovalCommand, DefaultApprovalChoices().
---
Outside diff comments:
In `@pkg/ai-stream/approval_command.go`:
- Around line 108-115: The update to pending.request in the PublishRequested
hook is not synchronized and can race with ResolveCommand reading
pending.request. Modify the code around hooks.PublishRequested (and the similar
block at lines 147-153) to acquire the command-level mutex (c.mu) before writing
pending.request (and keep it held while assigning the new request) or otherwise
perform the assignment inside the same critical section that calls add()/sets up
pending, ensuring pending.request updates and reads (e.g., in ResolveCommand)
are protected by the same c.mu lock; update both the PublishRequested paths and
any other places that mutate pending.request to use c.mu consistently.
- Around line 164-175: Resolve currently releases c.mu before sending to
pending.response, allowing Request to delete the entry in between; keep the
lookup and delivery atomic by holding c.mu while performing the non-blocking
send: inside Resolve, lock c.mu, load pending := c.pending[response.ID], if
pending == nil unlock and return false, then perform the non-blocking send to
pending.response (select { case pending.response <- response: success = true
default: success = false }), unlock c.mu and return success; refer to Resolve,
c.mu, c.pending, pending.response and Request/wait interactions when making the
change.
In `@pkg/connector/client.go`:
- Around line 2598-2613: The early return in waitForAssistantAnchorMessage when
stream.eventID is non-empty skips waiting for the anchor persistence and can
cause dropped final/error edits; remove that early return and always call
waitForMessageEventID (using stream.portal and stream.messageID with
aiid.PartID("text") and the same timeout) so the function ensures the anchor row
is editable even if eventID is already known; keep existing nil checks for
stream and stream.portal and still set stream.eventID from the returned eventID
if it was previously empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e86ff4b-bca4-4dd8-baf9-2de951b6139b
📒 Files selected for processing (7)
pkg/ai-stream/approval_command.gopkg/ai-stream/approval_command_test.gopkg/connector/approvals.gopkg/connector/client.gopkg/connector/slash_commands_approvals.gopkg/connector/slash_commands_test.gopkg/connector/stream_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ai-stream/approval_command_test.go
- pkg/connector/stream_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: build-docker
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
1 participant
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.
No description provided.