add ai streaming examples#8
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces a complete in-process AI demo runtime system integrated with the Matrix bridge. It adds command parsing for lorem/tools/random/chaos modes, deterministic execution engines with virtual timing, text generation with markdown safety, and reaction-based approval workflows. Member lookups migrate from slices to maps, containers upgrade to Go 1.25, and extensive tests validate parsing, event emission, approval continuations, and syntax safety. ChangesAI Streaming Demo & Connector Integration
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
pkg/ag-ui/events.go (1)
286-306: 💤 Low valueBuilder allows creating events that fail validation.
StepStartedandStepFinishedconditionally addstepNameonly when non-empty, butValidateEvent(line 424) requires it viarequire(evt, "stepName"). Callingbuilder.StepStarted("msg", "")produces an event that will fail validation.Consider requiring non-empty
stepNameor documenting that callers must provide it.🤖 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/ag-ui/events.go` around lines 286 - 306, The builder currently omits "stepName" when empty causing ValidateEvent (which calls require(evt, "stepName")) to fail; update EventBuilder.StepStarted and EventBuilder.StepFinished to require a non-empty stepName: add an explicit guard at the start of each function that checks stepName != "" and fails fast with a clear message (e.g., panic or log.Fatalf) if empty, then always assign evt["stepName"] = stepName before returning so ValidateEvent will pass.pkg/connector/ai_runtime_test.go (1)
432-479: ⚡ Quick winAdd a random-mode approval pause regression test.
Current random tests validate selection/terminal distribution, but not run-state semantics when approval is emitted. Add a case for
stream-random ... --allow-approvalthat asserts noRUN_FINISHEDandrun.Status.State == "streaming"once an approval request appears.🤖 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/ai_runtime_test.go` around lines 432 - 479, Add a new regression test (e.g., TestRandomModeApprovalPause) in pkg/connector/ai_runtime_test.go that constructs a randomCommand with sharedStreamOptions.AllowApproval = true (use randomCommand and randomActionToolApproval to detect when an approval is emitted), run the stream-random flow or simulate steps until an approval action is produced, and then assert that no RUN_FINISHED event was emitted and that the run object has run.Status.State == "streaming"; reference chooseRandomAction/randomCommand/randomActionToolApproval to detect approval and check for absence of RUN_FINISHED and the run.Status.State field to equal "streaming".
🤖 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/bridgev2/events.go`:
- Around line 90-91: In ApprovalOptionReaction, guard against an empty
option.Values before accessing option.Values[0]; update the code that constructs
the reaction (the block setting EmojiID and Emoji) to check len(option.Values)>0
and only set Emoji/EmojiID from option.Values[0] when present, otherwise set a
safe default (e.g., empty string) or skip adding the Emoji fields so no panic
occurs when Values is empty.
In `@pkg/ai-stream/matrix/content.go`:
- Around line 19-22: The code sets rendered.Format = event.FormatHTML and copies
rendered.Body into rendered.FormattedBody without escaping, which allows raw
markup to be rendered unsafely; update the fallback in
pkg/ai-stream/matrix/content.go (the block that checks if rendered.Format !=
event.FormatHTML) to assign an HTML-escaped version of rendered.Body to
rendered.FormattedBody (e.g., use html.EscapeString(rendered.Body) or your
project's HTML sanitizer), and add the required import for the html package (or
call the existing sanitizer function) so AnchorContent/Rendered content cannot
inject unsanitized HTML.
In `@pkg/ai-stream/pack.go`:
- Around line 307-311: The current block conflates json.Marshal errors with size
truncation; update the cp["rawEvent"] handling so that if
json.Marshal(cp["rawEvent"]) returns an error you remove cp["rawEvent"] (delete
from the map) and do not set a truncated value, and only when marshal succeeds
inspect len(raw) and, if >2048, set cp["rawEvent"] to the truncated string and
set cp["rawEventTruncated"]=true; reference the json.Marshal call, the raw
variable, and the cp map (cp["rawEvent"], cp["rawEventTruncated"]) when making
the change.
In `@pkg/connector/ai_runtime.go`:
- Around line 779-787: The random-mode branches currently discard the error from
r.runToolSpec; capture the returned error in each case (for randomActionTool,
randomActionToolFail, randomActionToolDeny, randomActionToolApproval) and
propagate errApprovalRequested instead of dropping it so approval flows can
pause correctly (e.g., err := r.runToolSpec(...); if errors.Is(err,
errApprovalRequested) { return err } else /* optionally log or ignore other
errors */). Ensure you import/use errors.Is if needed and keep the same
SequenceIndex/rng/defaultCommonOptions usage when calling runToolSpec.
In `@pkg/connector/ai_text.go`:
- Around line 184-193: The loop that builds `kept` only breaks when `next >
limit && len(kept) > 0`, which lets an oversized first markdown `block` exceed
`limit`; fix by enforcing a hard cap when the first block alone goes over the
budget: inside the loop in the function using `kept`, `block`, `next`, `total`,
and `limit`, change the `if next > limit` branch so that if `len(kept) > 0` you
break as before, but if `len(kept) == 0` you trim the current `block` to `limit`
(use the existing `trimText` helper) and append/return that trimmed block
immediately (or set `kept` to the trimmed block and break) so the returned
string from `strings.Join(kept, "\n\n")` cannot exceed `limit`.
In `@pkg/connector/client.go`:
- Around line 483-486: The goroutine started with
dc.queueAIRunStreamAndMetadata(portal, placeholderID, *plan.Run) is not tracked
by dc.wg and may outlive Disconnect; fix by following the same pattern used in
queueRemoteEcho: call dc.wg.Add(1) immediately before spawning the goroutine and
ensure the goroutine calls dc.wg.Done() when it exits (either via a defer at the
top of the goroutine body or by wrapping the call in an inline function that
defers Done). This ensures dc.wg.Wait() in Disconnect() will wait for
dc.queueAIRunStreamAndMetadata to finish.
In `@TANSTACK_AG_UI_PARITY_PLAN.md`:
- Around line 140-217: The "Full Paths To Inspect" section in
TANSTACK_AG_UI_PARITY_PLAN.md contains machine-specific absolute paths (e.g.,
/Users/batuhan/Projects/...) that leak user/workspace details and reduce
portability; update that section to use repo-relative paths and generic
placeholders (for example replace "/Users/batuhan/Projects/labs/dummybridge"
with "repo-root/dummybridge" or "<local-path>/dummybridge") and remove any
personal usernames, keeping the same list items and filenames but ensuring all
entries are non-absolute and generic; focus your edits inside the "Full Paths To
Inspect" block so symbols like the listed directories and filenames remain but
are expressed as relative paths or placeholders.
---
Nitpick comments:
In `@pkg/ag-ui/events.go`:
- Around line 286-306: The builder currently omits "stepName" when empty causing
ValidateEvent (which calls require(evt, "stepName")) to fail; update
EventBuilder.StepStarted and EventBuilder.StepFinished to require a non-empty
stepName: add an explicit guard at the start of each function that checks
stepName != "" and fails fast with a clear message (e.g., panic or log.Fatalf)
if empty, then always assign evt["stepName"] = stepName before returning so
ValidateEvent will pass.
In `@pkg/connector/ai_runtime_test.go`:
- Around line 432-479: Add a new regression test (e.g.,
TestRandomModeApprovalPause) in pkg/connector/ai_runtime_test.go that constructs
a randomCommand with sharedStreamOptions.AllowApproval = true (use randomCommand
and randomActionToolApproval to detect when an approval is emitted), run the
stream-random flow or simulate steps until an approval action is produced, and
then assert that no RUN_FINISHED event was emitted and that the run object has
run.Status.State == "streaming"; reference
chooseRandomAction/randomCommand/randomActionToolApproval to detect approval and
check for absence of RUN_FINISHED and the run.Status.State field to equal
"streaming".
🪄 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: 14d0c592-cdd1-4759-9a2d-34e2707d00e3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
TANSTACK_AG_UI_PARITY_PLAN.mdgo.modpkg/ag-ui/events.gopkg/ag-ui/events_test.gopkg/ai-stream/approval.gopkg/ai-stream/bridgev2/events.gopkg/ai-stream/bridgev2/events_test.gopkg/ai-stream/matrix/content.gopkg/ai-stream/matrix/content_test.gopkg/ai-stream/pack.gopkg/ai-stream/run.gopkg/ai-stream/stream_test.gopkg/connector/ai_runtime.gopkg/connector/ai_runtime_test.gopkg/connector/ai_text.gopkg/connector/client.gopkg/connector/client_test.gopkg/connector/connector.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-dummybridge
🧰 Additional context used
🪛 LanguageTool
TANSTACK_AG_UI_PARITY_PLAN.md
[style] ~30-~30: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...unsupported or failed merge behavior. - No final full-text Matrix edit for long ru...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~33-~33: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s already describe the part contract. - No package-manager install/update/lockfile...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~75-~75: Use a hyphen to join words.
Context: ... ./...in dummybridge passes. - Desktop focused AI/PAS tests pass: -bun run ...
(QB_NEW_EN_HYPHEN)
[style] ~91-~91: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...review instead of reverting to .... - Live staging random/chaos smoke passes for h...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~263-~263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...START.index` for parallel tool calls. - Support partial JSON argument streaming through...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~264-~264: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...fort and finalize on TOOL_CALL_END. - Support TOOL_CALL_END both with and without a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~265-~265: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...th with and without a result payload. - Support AG-UI TOOL_CALL_RESULT for separate t...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~266-~266: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... of Beeper custom tool-result events. - Support multiple assistant messageIds per run...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~355-~355: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t, and tool failures. - stream-random emits weighted random actions with determinis...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~365-~365: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...um chunk size option: 512 characters. - Archived maximum random actions: 64. - Archived ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~366-~366: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Archived maximum random actions: 64. - Archived maximum chaos runs: 16. - Archived maxi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~367-~367: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...4. - Archived maximum chaos runs: 16. - Archived maximum chaos actions: 64. - Archived m...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~368-~368: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...- Archived maximum chaos actions: 64. - Archived maximum demo duration: 5 minutes. - Arc...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~369-~369: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ved maximum demo duration: 5 minutes. - Archived maximum delay/stagger: 30 seconds. - In...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~534-~534: To form a complete sentence, be sure to include a subject.
Context: ... primitive. Run errors: - RUN_ERROR may be run-scoped or session/thread-scoped....
(MISSING_IT_THERE)
[style] ~621-~621: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ead of maintaining a separate parser. - Use @tanstack/ai-react-ui for `ChatMessag...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~781-~781: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... command guide includes all commands. - Verify conflicting terminal options are reject...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~782-~782: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...icting terminal options are rejected. - Verify invalid random profile is rejected. - V...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~783-~783: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y invalid random profile is rejected. - Verify oversized option inputs are rejected. -...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~784-~784: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...oversized option inputs are rejected. - Verify markdown-rich text generation is determ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~785-~785: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...stic by seed and varied across calls. - Verify table/link/list/code/quote markdown sig...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~786-~786: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ink/list/code/quote markdown signals. - Verify stream-lorem emits thinking, steps, t...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~787-~787: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s transient data from final snapshot. - Verify stream-tools covers success, failure,...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~788-~788: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... output, and provider-executed tools. - Verify random streams finish and respect durat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~789-~789: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... streams finish and respect duration. - Verify chaos streams start multiple runs with ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~790-~790: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...le runs with stagger and max-actions. - Verify error and abort terminal states. `pkg/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~796-~796: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...date event builders and required IDs. - Validate RunAgentInput. - Validate UIMessage...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~797-~797: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ired IDs. - Validate RunAgentInput. - Validate UIMessage ordered part shape. - Valid...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~798-~798: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...idate UIMessage ordered part shape. - Validate tool-call and tool-result states agains...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~799-~799: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...esult states against TanStack values. - Validate approval request and response shapes. -...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~800-~800: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...approval request and response shapes. - Validate step/thinking events. - Validate `STATE...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~801-~801: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pes. - Validate step/thinking events. - Validate STATE_SNAPSHOT, STATE_DELTA, and `M...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~802-~802: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...and MESSAGES_SNAPSHOT event shapes. - Validate every emitted event has timestamp. - ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~803-~803: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... every emitted event has timestamp. - Validate rawEvent is optional and bounded/trun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~804-~804: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tted before exceeding carrier limits. - Validate TOOL_CALL_START.index. - Validate par...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~805-~805: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .... - Validate TOOL_CALL_START.index. - Validate partial JSON TOOL_CALL_ARGS. - Valida...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~806-~806: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lidate partial JSON TOOL_CALL_ARGS. - Validate TOOL_CALL_END with and without result...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~807-~807: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...OL_CALL_ENDwith and without result. - ValidateTOOL_CALL_RESULT` creates/updates TanS...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~808-~808: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...updates TanStack tool-result parts. - Validate run-scoped and thread/session-scoped `R...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~809-~809: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nd thread/session-scoped RUN_ERROR. - Validate multiple assistant messageIds per run...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~816-~816: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nalization totals such as seqTotal. - Verify no per-delta accumulated full text. - V...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~818-~818: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mulator is only used at finalization. - Verify UTF-8 splitting. - Verify carrier packe...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~820-~820: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s the serialized JSON carrier budget. - Verify stream reconstruction from carriers. - ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~821-~821: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... stream reconstruction from carriers. - Verify finalization carriers split a complete ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~822-~822: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ion parts without repeating metadata. - Verify finalization continuations merge determ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~823-~823: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...essageId, runId, and partOffset`. - Verify oversized text/thinking final parts spl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~824-~824: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-8 boundaries and reassemble exactly. - Verify oversized raw/debug/provider metadata i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~825-~825: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...splitting structured tool/data parts. - Verify duplicate/stale/out-of-order seq beha...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~826-~826: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...te/stale/out-of-order seq behavior. - Verify missing seq gap timeout marks the anc...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~827-~827: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t marks the anchor incomplete/failed. - Verify carrier delete/redaction recomputes or ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~835-~835: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...least two carrier events are emitted. - Assert later carriers have no preview body or ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~836-~836: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...no preview body or only minimal body. - Assert reconstruction from deltas exactly equa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~837-~837: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ltas exactly equals generated output. - Assert no final full-body edit is required to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~838-~838: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...uired to display the complete stream. - Assert final snapshot state is complete even w...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~895-~895: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...AI message, not many carrier bubbles. - Send `stream-tools 200 shell#approval --seed...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (58)
go.mod (1)
3-3: LGTM!Also applies to: 8-10, 14-14, 16-17, 20-21, 24-24, 28-28, 30-35
pkg/connector/connector.go (1)
46-56: LGTM!Also applies to: 72-77
pkg/ai-stream/pack.go (7)
1-29: LGTM!
31-52: LGTM!
54-94: LGTM!
112-139: LGTM!
141-181: LGTM!
183-266: LGTM!
319-325: LGTM!pkg/ai-stream/stream_test.go (6)
1-54: LGTM!
56-145: LGTM!
147-209: LGTM!
211-257: LGTM!
259-333: LGTM!
335-346: LGTM!pkg/ag-ui/events.go (9)
1-57: LGTM!
59-115: LGTM!
117-165: LGTM!
167-239: LGTM!
241-284: LGTM!
308-358: LGTM!
360-465: LGTM!
467-559: LGTM!
561-650: LGTM!pkg/ag-ui/events_test.go (4)
1-62: LGTM!
64-85: LGTM!
87-121: LGTM!
123-144: LGTM!pkg/ai-stream/run.go (7)
1-77: LGTM!
79-160: LGTM!
162-256: LGTM!
258-341: LGTM!
343-501: LGTM!
503-567: LGTM!
569-658: LGTM!pkg/ai-stream/approval.go (3)
1-51: LGTM!
53-120: LGTM!
122-197: LGTM!pkg/connector/client.go (12)
3-54: LGTM!
143-197: LGTM!
199-250: LGTM!
252-267: LGTM!
269-303: LGTM!
305-370: LGTM!
372-381: LGTM!
495-529: LGTM!
531-582: LGTM!
584-630: LGTM!
632-740: LGTM!
760-890: LGTM!pkg/connector/client_test.go (4)
3-14: LGTM!
47-57: LGTM!
59-79: LGTM!
81-115: LGTM!pkg/ai-stream/matrix/content.go (1)
38-94: LGTM!pkg/ai-stream/matrix/content_test.go (1)
14-131: LGTM!pkg/ai-stream/bridgev2/events.go (1)
17-79: LGTM!Also applies to: 103-133
pkg/ai-stream/bridgev2/events_test.go (1)
15-100: LGTM!
|
@indent review |
|
|
Done — summary and findings posted in the banner comment. The headline items worth a closer look:
|
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/connector/client.go`:
- Around line 342-347: The synthetic live reaction event is using the Matrix
MXID (string(msg.Event.Sender)) for Sender, but stored approvals use the
normalized bridge sender ID (Reaction.SenderID / produced by
PreHandleMatrixReaction), causing CleanupReactions to treat them as different
senders; update the aistream.ReactionEvent construction in
pkg/connector/client.go to set Sender to the normalized bridge sender ID (use
the Reaction.SenderID from the stored reaction or the output of
PreHandleMatrixReaction where reactions are normalized) instead of
msg.Event.Sender so live events and stored approvals use the same sender
identifier.
- Around line 295-300: The success log unconditionally dereferences msg.Event
(Stringer("sender", msg.Event.Sender)) and can panic when msg.Event is nil;
update the logging in the same scope (cleanupApprovalReactions) to guard
msg.Event first—either omit or provide a safe fallback for the "sender" field
when msg.Event == nil (e.g., log a nil/unknown sender value or use a conditional
Stringer only when msg.Event != nil) while keeping the other fields (approvalID,
reaction, selected.Value.Approved) the same.
- Around line 842-867: Replace the deprecated Members slice with the MemberMap
on the ChatMemberList so the AI participants are actually registered: in the
code that constructs the bridgev2.ChatMemberList (currently setting Members:
[]bridgev2.ChatMember{...}), build MemberMap instead and populate entries keyed
by each member ID (use networkid.UserID(dc.UserLogin.ID) and aiGhostID as keys)
with corresponding bridgev2.ChatMember values (including EventSender,
Membership, PowerLevel and MemberEventExtra for the AI ghost using aiGhostName
and aiGhostID). Ensure the map keys and types match
bridgev2.ChatMemberList.MemberMap so the framework in v0.28.0 will see the
participants.
🪄 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: 53896c9a-5ced-43a3-8eef-443cec0f1337
📒 Files selected for processing (4)
pkg/connector/client.gopkg/connector/commands.gopkg/connector/generators.gopkg/connector/message_requests.go
✅ Files skipped from review due to trivial changes (1)
- pkg/connector/commands.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-dummybridge
- GitHub Check: build-docker-dummybridge
🔇 Additional comments (2)
pkg/connector/message_requests.go (1)
110-122: LGTM!Also applies to: 133-136
pkg/connector/generators.go (1)
55-71: LGTM!Also applies to: 81-86
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/dummybridge/Dockerfile (1)
10-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the runtime container as a non-root user.
The final image has no
USERdirective, so the process runs as root.Suggested fix
FROM alpine:3.20 RUN apk add --no-cache ca-certificates olm su-exec bash jq yq curl -COPY --from=builder /usr/bin/dummybridge /usr/bin/dummybridge +RUN addgroup -S app && adduser -S -G app app +COPY --from=builder --chown=app:app /usr/bin/dummybridge /usr/bin/dummybridge +USER app:app CMD ["/usr/bin/dummybridge"]🤖 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 `@cmd/dummybridge/Dockerfile` around lines 10 - 14, The Dockerfile currently runs the container as root because it lacks a USER directive; modify the Dockerfile to create a non-root user (e.g., "appuser"), ensure /usr/bin/dummybridge is owned by that user (chown) and any needed directories are accessible, and add a USER appuser line before CMD so the process launched by CMD ["/usr/bin/dummybridge"] runs unprivileged; reference the COPY --from=builder /usr/bin/dummybridge and CMD lines when applying these changes.cmd/loginhelper/Dockerfile (2)
10-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the runtime container as a non-root user.
The final image has no
USERdirective, so the process runs as root.Suggested fix
FROM alpine:3.20 RUN apk add --no-cache ca-certificates -COPY --from=builder /usr/bin/loginhelper /usr/bin/loginhelper +RUN addgroup -S app && adduser -S -G app app +COPY --from=builder --chown=app:app /usr/bin/loginhelper /usr/bin/loginhelper +USER app:app CMD ["/usr/bin/loginhelper"]🤖 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 `@cmd/loginhelper/Dockerfile` around lines 10 - 14, The Dockerfile runs /usr/bin/loginhelper as root; add a non-root runtime user and switch to it: create a dedicated user and group (e.g., loginhelper), ensure /usr/bin/loginhelper is owned by that user (chown) and has appropriate permissions, and add a USER loginhelper directive before CMD to run the process unprivileged; reference the Dockerfile entries COPY --from=builder /usr/bin/loginhelper /usr/bin/loginhelper and CMD ["/usr/bin/loginhelper"] when making these changes.
1-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign builder and runtime Alpine versions when CGO is enabled.
Line 1 builds on
alpine3.23, but Line 10 runs onalpine:3.20while Line 7 setsCGO_ENABLED=1. This creates a libc/ABI mismatch between build and runtime images that can break startup.Suggested fix
-FROM alpine:3.20 +FROM alpine:3.23🤖 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 `@cmd/loginhelper/Dockerfile` around lines 1 - 10, The runtime and builder Alpine versions mismatch while CGO is enabled (CGO_ENABLED=1) which can cause libc/ABI issues; update the Dockerfile so the final image uses the same Alpine release as the builder (or alternatively disable CGO) — specifically, make the final FROM line match the builder stage (alpine3.23) or change the builder to alpine:3.20 so the build stage (where CGO_ENABLED=1 and go build -o /usr/bin/loginhelper ./cmd/loginhelper runs) and the runtime image use the same libc/ABI.
🤖 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.
Outside diff comments:
In `@cmd/dummybridge/Dockerfile`:
- Around line 10-14: The Dockerfile currently runs the container as root because
it lacks a USER directive; modify the Dockerfile to create a non-root user
(e.g., "appuser"), ensure /usr/bin/dummybridge is owned by that user (chown) and
any needed directories are accessible, and add a USER appuser line before CMD so
the process launched by CMD ["/usr/bin/dummybridge"] runs unprivileged;
reference the COPY --from=builder /usr/bin/dummybridge and CMD lines when
applying these changes.
In `@cmd/loginhelper/Dockerfile`:
- Around line 10-14: The Dockerfile runs /usr/bin/loginhelper as root; add a
non-root runtime user and switch to it: create a dedicated user and group (e.g.,
loginhelper), ensure /usr/bin/loginhelper is owned by that user (chown) and has
appropriate permissions, and add a USER loginhelper directive before CMD to run
the process unprivileged; reference the Dockerfile entries COPY --from=builder
/usr/bin/loginhelper /usr/bin/loginhelper and CMD ["/usr/bin/loginhelper"] when
making these changes.
- Around line 1-10: The runtime and builder Alpine versions mismatch while CGO
is enabled (CGO_ENABLED=1) which can cause libc/ABI issues; update the
Dockerfile so the final image uses the same Alpine release as the builder (or
alternatively disable CGO) — specifically, make the final FROM line match the
builder stage (alpine3.23) or change the builder to alpine:3.20 so the build
stage (where CGO_ENABLED=1 and go build -o /usr/bin/loginhelper
./cmd/loginhelper runs) and the runtime image use the same libc/ABI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97e2537e-25bb-46a0-9faf-f21c59537950
📒 Files selected for processing (5)
cmd/dummybridge/Dockerfilecmd/loginhelper/Dockerfilepkg/ai-stream/matrix/content.gopkg/connector/ai_runtime.gopkg/connector/client.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/ai-stream/matrix/content.go
- pkg/connector/client.go
- pkg/connector/ai_runtime.go
📜 Review details
🧰 Additional context used
🪛 Trivy (0.69.3)
cmd/loginhelper/Dockerfile
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
cmd/dummybridge/Dockerfile
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🔇 Additional comments (1)
cmd/dummybridge/Dockerfile (1)
1-1: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/client.go (1)
444-460:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
streammessages to parse before opting into the demo flow.This predicate now fires for any message whose first token is
stream, so normal-room text likestream of consciousnesswill queue an unsolicited AI help/error response. Gate thestream/stream-toolsbranches onparseCommandsucceeding instead of matching only the first token.Possible fix
func isAIDemoCommandContent(content *event.MessageEventContent) bool { if content == nil { return false } - tokens := strings.Fields(strings.TrimSpace(content.Body)) + body := strings.TrimSpace(content.Body) + tokens := strings.Fields(body) if len(tokens) == 0 { return false } switch strings.ToLower(tokens[0]) { - case "help", "/help", "!help", "stream", "stream-tools": + case "help", "/help", "!help": return true + case "stream", "stream-tools": + _, err := parseCommand(body) + return err == nil case "dummybridge": return len(tokens) > 1 && strings.EqualFold(tokens[1], "help") 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/connector/client.go` around lines 444 - 460, isAIDemoCommandContent currently treats any message starting with "stream" or "stream-tools" as a demo trigger; change it to call parseCommand(content) and only return true for the "stream"/"stream-tools" branches when parseCommand succeeds (i.e., parsed command is non-nil/successful), while keeping existing behavior for "help"/"/help"/"!help" and the "dummybridge help" check; locate the isAIDemoCommandContent function and add a parseCommand(content) check before accepting the "stream" or "stream-tools" tokens.
🤖 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/connector/ai_runtime.go`:
- Around line 349-377: The current buildAIStreamRunPlans function samples an
independent delay for each child causing later runs to possibly start earlier
than previous ones; change it to accumulate stagger offsets so delays are
cumulative: introduce a cumulativeOffset (time.Duration) initialized zero, for
each child call aiRunner{runtime: virtualAIRuntime(now)}.sampleDelay(...) to get
nextDelta, add nextDelta to cumulativeOffset, use cumulativeOffset when
computing run start time (now.Add(cumulativeOffset)) and set
plan.Delay=cumulativeOffset (instead of the single-sample delay); keep other
fields (child.Seed, parsed, buildAIRunFromCommand, EffectiveCommand) the same so
queueAIResponse sees delays as "wait this long from now" in increasing order.
In `@pkg/connector/client.go`:
- Around line 633-635: emitAIRunStream is eagerly creating entries in
dc.aiRunSessions via ensureAIRunSession which permanently retains a session per
streamed run; remove that eager allocation by not calling ensureAIRunSession
from emitAIRunStream and instead only lookup the session (e.g. check existence
without creating) so sessions are only created by recordAIApprovalDecision when
an approval continuation is required; apply the same change to the other
occurrences referenced (the blocks around the other noted ranges) so no code
path that merely streams a run creates a session.
---
Outside diff comments:
In `@pkg/connector/client.go`:
- Around line 444-460: isAIDemoCommandContent currently treats any message
starting with "stream" or "stream-tools" as a demo trigger; change it to call
parseCommand(content) and only return true for the "stream"/"stream-tools"
branches when parseCommand succeeds (i.e., parsed command is
non-nil/successful), while keeping existing behavior for "help"/"/help"/"!help"
and the "dummybridge help" check; locate the isAIDemoCommandContent function and
add a parseCommand(content) check before accepting the "stream" or
"stream-tools" tokens.
🪄 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: f43a2847-d37a-4a6c-9821-498d78971ded
📒 Files selected for processing (8)
pkg/ai-stream/matrix/content.gopkg/ai-stream/matrix/content_test.gopkg/ai-stream/run.gopkg/connector/ai_runtime.gopkg/connector/ai_runtime_test.gopkg/connector/ai_stream_random.gopkg/connector/client.gopkg/connector/client_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-dummybridge
- GitHub Check: build-docker-dummybridge
|
@indent rereview |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/connector/ai_runtime.go`:
- Around line 357-360: The sampled per-run Delay is currently computed
independently from now causing later runs to possibly start before earlier ones;
change the logic in the loop that calls aiRunner{runtime:
virtualAIRuntime(now)}.sampleDelay(...) to accumulate delays: track an
accumulatedDelay (time.Duration) and for each child run add the sampled delay to
accumulatedDelay (or advance the base time passed to virtualAIRuntime by
accumulatedDelay) so that run N’s start = now + sum(delays[0..N]); apply the
same cumulative fix to the other block referenced around aiRunner.sampleDelay at
lines ~370-373 to ensure all staggered multi-run streams are strictly
sequential.
In `@pkg/connector/ai_text.go`:
- Around line 183-185: The code currently returns synthetic lorem via
buildLoremText when isMarkdownSensitiveBlock(block) and the first block exceeds
limit; instead, preserve the original content by trimming the block to the
requested limit (rune-aware) and return the truncated block so markdown meaning
is retained; remove the buildLoremText/rand.New path and replace it with logic
that truncates block (use limit on runes/bytes as appropriate) and returns the
original block content truncated, keeping any necessary markdown delimiters
intact (refer to isMarkdownSensitiveBlock, buildLoremText, and limit to locate
and change the behavior).
In `@pkg/connector/client.go`:
- Around line 645-653: The code is assigning the same continuation sequence
(nextSeq) to every approval prompt, causing overlapping sequence ranges; change
the logic in the block that iterates run.Prompts so each prompt gets a unique
SeqStart before calling dc.queueAIApprovalPrompt: for each prompt compute and
assign prompt.SeqStart = nextSeq, then reserve/increment nextSeq by the expected
sequence length for that prompt (e.g., prompt payload length or a computed
seqRangePerPrompt) so subsequent prompts get disjoint ranges, or alternatively
serialize by only queueing one prompt and deferring others; update the same loop
around run.Prompts and the use of nextSeq and queueAIApprovalPrompt accordingly.
🪄 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: a9ff5c96-f5f0-41ee-965e-5f45d8f5b791
📒 Files selected for processing (11)
pkg/ai-stream/approval.gopkg/ai-stream/matrix/content.gopkg/ai-stream/matrix/content_test.gopkg/ai-stream/run.gopkg/ai-stream/stream_test.gopkg/connector/ai_runtime.gopkg/connector/ai_runtime_test.gopkg/connector/ai_stream_random.gopkg/connector/ai_text.gopkg/connector/client.gopkg/connector/client_test.go
📜 Review details
🔇 Additional comments (1)
pkg/connector/client.go (1)
634-635:emitAIRunStreamstill retains a session for every streamed run.
ensureAIRunSession(run.RunID)is still unconditional here, so runs that never request approval still leave permanent entries indc.aiRunSessions.
|
Re-review posted. Highlights: Verified resolved on New findings (not covered by CodeRabbit):
CodeRabbit's still-open findings (verified, not duplicated): |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/connector/ai_runtime.go (1)
356-360:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--stagger-msis not cumulative across child stream runs.Each run delay is currently sampled from zero, so run N can start before run N-1. Delays should accumulate to preserve ordering.
Proposed fix
rng := rand.New(rand.NewSource(seed)) plans := make([]aiRunPlan, 0, cmd.Runs) + var offset time.Duration for i := range cmd.Runs { - var delay time.Duration if i > 0 { - delay = aiRunner{runtime: virtualAIRuntime(now)}.sampleDelay(rng, cmd.StaggerMin, cmd.StaggerMax) + offset += aiRunner{runtime: virtualAIRuntime(now)}.sampleDelay(rng, cmd.StaggerMin, cmd.StaggerMax) } + delay := offset child := cmd child.Runs = 1Also applies to: 370-373
🤖 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/ai_runtime.go` around lines 356 - 360, The per-run delay sampling in the loop over cmd.Runs currently assigns each run a delay independently, allowing run N to start before run N-1; change it to accumulate delays so each run's start is offset by the sum of prior delays. Keep using aiRunner{runtime: virtualAIRuntime(now)}.sampleDelay(rng, cmd.StaggerMin, cmd.StaggerMax) to sample the incremental delay, but add it to a running accumulator (e.g., totalDelay) and assign that accumulated value to each run instead of the fresh sample; apply the same accumulation fix to the similar block referenced around lines 370-373 so both child stream run loops preserve ordering.pkg/connector/ai_text.go (1)
183-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not replace oversized first markdown block with synthetic lorem.
When the first block exceeds
limit, this branch discards the original content and returns unrelated text. Trim the original block instead so output meaning is preserved.Proposed fix
if next > limit { if len(kept) == 0 { - if isMarkdownSensitiveBlock(block) { - return buildLoremText(limit, rand.New(rand.NewSource(int64(limit)))) - } kept = append(kept, trimText(block, limit)) } break }🤖 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/ai_text.go` around lines 183 - 185, The current branch in the code that checks isMarkdownSensitiveBlock(block) returns buildLoremText(limit, ...) when the first markdown block exceeds limit, which discards original content; instead, modify the logic in the block handling (where isMarkdownSensitiveBlock and buildLoremText are used) to trim the original block to the requested limit and return the trimmed content (preserving formatting) rather than generating synthetic lorem; ensure trimming respects markup boundaries and uses the same limit variable so callers receive a shortened version of the real block rather than unrelated text.
🤖 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/connector/client.go`:
- Around line 678-685: The current branch aborts the run when
aistream.NextSeq(carriers) yields actualNextSeq != nextSeq; instead of
returning, update the run's sequence assignments by recomputing SeqStart offsets
for the queued approval prompts from the repacked carriers (or reserve
sufficient sequence space earlier) so stream metadata stays consistent; locate
the check around aistream.NextSeq(carriers) / actualNextSeq / nextSeq and
replace the early return with logic that logs the discrepancy (keeping the
existing warn) then recalculates and applies new SeqStart values for the pending
approvals (or expands the reserved sequence range) before continuing to process
the stream and emit final metadata for run.RunID.
---
Duplicate comments:
In `@pkg/connector/ai_runtime.go`:
- Around line 356-360: The per-run delay sampling in the loop over cmd.Runs
currently assigns each run a delay independently, allowing run N to start before
run N-1; change it to accumulate delays so each run's start is offset by the sum
of prior delays. Keep using aiRunner{runtime:
virtualAIRuntime(now)}.sampleDelay(rng, cmd.StaggerMin, cmd.StaggerMax) to
sample the incremental delay, but add it to a running accumulator (e.g.,
totalDelay) and assign that accumulated value to each run instead of the fresh
sample; apply the same accumulation fix to the similar block referenced around
lines 370-373 so both child stream run loops preserve ordering.
In `@pkg/connector/ai_text.go`:
- Around line 183-185: The current branch in the code that checks
isMarkdownSensitiveBlock(block) returns buildLoremText(limit, ...) when the
first markdown block exceeds limit, which discards original content; instead,
modify the logic in the block handling (where isMarkdownSensitiveBlock and
buildLoremText are used) to trim the original block to the requested limit and
return the trimmed content (preserving formatting) rather than generating
synthetic lorem; ensure trimming respects markup boundaries and uses the same
limit variable so callers receive a shortened version of the real block rather
than unrelated text.
🪄 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: ec6b85c2-8995-47ba-9f3a-60f090ee6908
📒 Files selected for processing (11)
pkg/ai-stream/approval.gopkg/ai-stream/matrix/content.gopkg/ai-stream/matrix/content_test.gopkg/ai-stream/run.gopkg/ai-stream/stream_test.gopkg/connector/ai_runtime.gopkg/connector/ai_runtime_test.gopkg/connector/ai_stream_random.gopkg/connector/ai_text.gopkg/connector/client.gopkg/connector/client_test.go
📜 Review details
🔇 Additional comments (1)
pkg/connector/ai_runtime_test.go (1)
771-774: Integer-range loop compatibility: no action needed
go.modconfigures Go1.25.0, which supportsfor range 400/for range 80inpkg/connector/ai_runtime_test.go(lines 771, 787, 800, 815).
|
@indent Fixed the remaining issue from your banner comment in
|
|
Verified |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/connector/client.go`:
- Around line 449-461: The current match for bare "help" triggers AI responses
in normal rooms; restrict bare "help" to AI portals by checking the channel
context. Update the help-handling branch (the tokens/tokens[0] switch in
pkg/connector/client.go) so that plain "help" only returns true when the message
originates from an AI portal (use or add a helper like isAIPortal(channelID) or
isPortal(ctx)); in non-portal channels only treat explicit prefixed forms (e.g.,
"/help" or "!help") as true. Keep the existing parseCommand usage for
"stream"/"stream-tools" and the "dummybridge" condition unchanged.
🪄 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: 503e6f32-af3c-4865-bc17-8bcb34845a6f
📒 Files selected for processing (3)
pkg/connector/ai_runtime.gopkg/connector/ai_text.gopkg/connector/client.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-dummybridge
- GitHub Check: build-docker-dummybridge
🔇 Additional comments (3)
pkg/connector/ai_runtime.go (1)
315-318: LGTM!Also applies to: 356-360
pkg/connector/ai_text.go (1)
184-186: LGTM!Also applies to: 209-229
pkg/connector/client.go (1)
402-406: LGTM!Also applies to: 648-656, 692-695, 1238-1247
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@go.mod`:
- Line 15: The go.mod entry currently pins github.com/beeper/ai-bridge to a
pseudo-version (6f18f21c0e09) but the code in pkg/connector/client.go and
pkg/connector/ai_runtime.go imports it directly, so either update the go.mod to
a tagged release (preferably v0.0.3) to ensure reproducible builds or, if that
specific commit is required, replace the pseudo-version with a short explainers
comment in go.mod/README describing why the untagged commit is necessary and
keep the pseudo-version; do not mark it "// indirect" because it is imported by
non-test code.
🪄 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: eda050a1-0017-4eca-80ab-1f0f13dbb188
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modpkg/connector/ai_runtime.gopkg/connector/ai_runtime_test.gopkg/connector/client.gopkg/connector/client_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-dummybridge
- GitHub Check: build-docker-dummybridge
🔇 Additional comments (43)
pkg/connector/client.go (22)
449-465: Barehelpstill hijacks plain messages in normal rooms.This was flagged in a previous review: plain
help//help/!helpin any chat now enqueues an AI response unexpectedly. The suggested fix was to keep bare help inside AI portals only, or require an explicit prefix likedummybridge helpin normal rooms.
13-28: LGTM!
30-46: LGTM!
48-98: LGTM!
100-149: LGTM!
151-205: LGTM!
207-258: LGTM!
260-324: LGTM!
326-409: LGTM!
411-443: LGTM!
467-516: LGTM!
518-564: LGTM!
566-619: LGTM!
621-709: LGTM!
711-820: LGTM!
822-866: LGTM!
868-928: LGTM!
930-1006: LGTM!
1008-1151: LGTM!
1153-1213: LGTM!
1215-1266: LGTM!
1268-1345: LGTM!pkg/connector/client_test.go (10)
1-17: LGTM!
18-47: LGTM!
49-73: LGTM!
75-82: LGTM!
84-94: LGTM!
96-123: LGTM!
125-154: LGTM!
156-191: LGTM!
193-207: LGTM!
209-246: LGTM!pkg/connector/ai_runtime.go (5)
1-163: LGTM!
165-378: LGTM!
411-764: LGTM!
840-1095: LGTM!
1097-1404: LGTM!pkg/connector/ai_runtime_test.go (6)
18-65: LGTM!
67-660: LGTM!
202-450: LGTM!Also applies to: 904-1019
714-827: LGTM!
829-902: LGTM!
1021-1057: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/connector/ai_commands.go`:
- Around line 203-205: The boolean flag handling incorrectly accepts
value-bearing tokens (e.g., --no-approval=false) and silently inverts intent;
update the parser branches that set cmd.AllowApproval (case "no-approval") and
the other boolean cases referenced (around the similar switch blocks at the
other occurrences) to detect and reject any non-empty token value and return an
error instead of accepting it, i.e., if the parsed token for the flag includes
an explicit value, fail fast with a clear error mentioning the flag name rather
than assigning the boolean; apply the same check to the other flagged cases at
the noted locations so all boolean flags reject value-bearing forms.
In `@pkg/connector/ai_plans.go`:
- Line 111: The code forces a minimum of 3 actions which ignores --max-actions
values <3; update the computation of actions (variable "actions" where max(3,
min(cmd.MaxActions, int(cmd.Duration/time.Second))) is used) so it no longer
forces 3—either remove the outer max or use max(1, ...) to only ensure at least
one action; use cmd.MaxActions and int(cmd.Duration/time.Second) inside min as
before and replace the 3 with 1 (or remove the outer max) so --max-actions=1|2
is honored.
🪄 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: 76209194-3f3f-4d00-aa55-ebaa5dadc82d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modpkg/connector/ai_commands.gopkg/connector/ai_parse_helpers.gopkg/connector/ai_plans.gopkg/connector/ai_runner.gopkg/connector/ai_runtime.gopkg/connector/ai_types.go
💤 Files with no reviewable changes (1)
- pkg/connector/ai_runtime.go
✅ Files skipped from review due to trivial changes (2)
- pkg/connector/ai_types.go
- go.mod
📜 Review details
🔇 Additional comments (2)
pkg/connector/ai_parse_helpers.go (1)
13-177: LGTM!pkg/connector/ai_runner.go (1)
17-416: LGTM!
| case "no-approval": | ||
| cmd.AllowApproval = false | ||
| default: |
There was a problem hiding this comment.
Reject explicit values for boolean flags.
Line 203/313/351/389-style flags are treated as enabled even when passed as --flag=false because value-bearing tokens are not rejected. This can silently invert user intent. Please fail fast when these flags include a value.
Proposed fix
@@
case "no-approval":
+ if hasValue {
+ return nil, fmt.Errorf("%s does not take a value", token)
+ }
cmd.AllowApproval = false
@@
case "meta":
+ if hasValue {
+ return opts, fmt.Errorf("%s does not take a value", token)
+ }
opts.Meta = true
@@
case "abort":
+ if hasValue {
+ return opts, fmt.Errorf("%s does not take a value", token)
+ }
opts.Abort = true
case "error":
+ if hasValue {
+ return opts, fmt.Errorf("%s does not take a value", token)
+ }
opts.Error = true
@@
case "allow-abort":
+ if hasValue {
+ return false, fmt.Errorf("%s does not take a value", token)
+ }
opts.AllowAbort = true
case "allow-error":
+ if hasValue {
+ return false, fmt.Errorf("%s does not take a value", token)
+ }
opts.AllowError = trueAlso applies to: 313-315, 351-354, 389-392
🤖 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/ai_commands.go` around lines 203 - 205, The boolean flag
handling incorrectly accepts value-bearing tokens (e.g., --no-approval=false)
and silently inverts intent; update the parser branches that set
cmd.AllowApproval (case "no-approval") and the other boolean cases referenced
(around the similar switch blocks at the other occurrences) to detect and reject
any non-empty token value and return an error instead of accepting it, i.e., if
the parsed token for the flag includes an explicit value, fail fast with a clear
error mentioning the flag name rather than assigning the boolean; apply the same
check to the other flagged cases at the noted locations so all boolean flags
reject value-bearing forms.
| } | ||
| rng := rand.New(rand.NewSource(seed)) | ||
| runner := aiRunner{runtime: virtualAIRuntime(now)} | ||
| actions := max(3, min(cmd.MaxActions, int(cmd.Duration/time.Second))) |
There was a problem hiding this comment.
Honor --max-actions when it is below 3.
Line 111 currently forces at least 3 actions, so --max-actions=1|2 is ignored. This breaks command semantics and can produce unexpected extra events.
Proposed fix
- actions := max(3, min(cmd.MaxActions, int(cmd.Duration/time.Second)))
+ maxByDuration := int(cmd.Duration / time.Second)
+ if maxByDuration < 1 {
+ maxByDuration = 1
+ }
+ actions := min(cmd.MaxActions, maxByDuration)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| actions := max(3, min(cmd.MaxActions, int(cmd.Duration/time.Second))) | |
| maxByDuration := int(cmd.Duration / time.Second) | |
| if maxByDuration < 1 { | |
| maxByDuration = 1 | |
| } | |
| actions := min(cmd.MaxActions, maxByDuration) |
🤖 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/ai_plans.go` at line 111, The code forces a minimum of 3
actions which ignores --max-actions values <3; update the computation of actions
(variable "actions" where max(3, min(cmd.MaxActions,
int(cmd.Duration/time.Second))) is used) so it no longer forces 3—either remove
the outer max or use max(1, ...) to only ensure at least one action; use
cmd.MaxActions and int(cmd.Duration/time.Second) inside min as before and
replace the 3 with 1 (or remove the outer max) so --max-actions=1|2 is honored.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/connector/client.go (1)
1090-1102: 💤 Low valueConsider removing unused
ensureAIRunSessionfunction.This function appears to be dead code after the fix that removed eager session creation from
emitAIRunStream.recordAIApprovalDecisionnow handles lazy session creation directly.🤖 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 1090 - 1102, The ensureAIRunSession function appears unused now; remove the ensureAIRunSession method declaration from DummyClient and delete any remaining references to it (search for ensureAIRunSession), ensuring that lazy session creation is handled by recordAIApprovalDecision which should continue to initialize dc.aiRunSessions under dc.aiRunSessionsMu and create aiRunSession entries (map and Decisions) when needed; keep aiRunSessions, aiRunSessionsMu, and aiRunSession types intact and run tests to confirm no regressions.
🤖 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.
Nitpick comments:
In `@pkg/connector/client.go`:
- Around line 1090-1102: The ensureAIRunSession function appears unused now;
remove the ensureAIRunSession method declaration from DummyClient and delete any
remaining references to it (search for ensureAIRunSession), ensuring that lazy
session creation is handled by recordAIApprovalDecision which should continue to
initialize dc.aiRunSessions under dc.aiRunSessionsMu and create aiRunSession
entries (map and Decisions) when needed; keep aiRunSessions, aiRunSessionsMu,
and aiRunSession types intact and run tests to confirm no regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44c5d7b6-45d0-4725-88cd-cea8f8b183e8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
build.shgo.modpkg/connector/client.gopkg/connector/commands.gopkg/connector/connector.go
💤 Files with no reviewable changes (1)
- pkg/connector/connector.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-dummybridge
- GitHub Check: build-docker-dummybridge
🔇 Additional comments (17)
go.mod (1)
3-3: LGTM!Also applies to: 9-11, 15-15, 17-18, 21-23, 29-29, 31-36
build.sh (1)
1-4: LGTM!pkg/connector/commands.go (4)
32-34: LGTM!
270-292: LGTM!
294-318: LGTM!
320-329: LGTM!pkg/connector/client.go (11)
233-248: LGTM!
250-297: LGTM!
315-372: LGTM!
374-382: LGTM!
508-554: LGTM!
580-651: LGTM!
764-808: LGTM!
1019-1084: LGTM!
1104-1128: LGTM!
1148-1196: LGTM!
418-447: LGTM!Also applies to: 460-498, 653-762, 810-870, 1130-1146
1 participant
No description provided.