fix: include recent Slack notifications in agent context#2147
Merged
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR injects per-request dynamic system-context providers into agent sessions, composes base+dynamic system prompts per LLM request, threads the provider through session creation/reactivation, and adds Slack bot code to record recent gateway events and attach them as dynamic system context for incoming Slack messages. ChangesDynamic System Context Infrastructure
Slack Recent Gateway Events
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
internal/agent/api_test.go (1)
700-733: ⚡ Quick winAdd a no-context follow-up assertion to prevent stale dynamic context regressions.
After the
run-2enqueue, enqueue once more with plaincontext.Background()and assertrun-2is not present in the next system message. This will lock in the intended “per-request volatile” behavior.🤖 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 `@internal/agent/api_test.go` around lines 700 - 733, Add a second enqueue using plain context.Background() after the existing run-2 test to verify dynamic system context is per-request: call api.EnqueueChatMessage(context.Background(), sessionID, user, ChatRequest{Message: "もう一度"}), wait for the next provider request via waitForRequest(reqCh,...), then assert req.Messages[0].Role is llm.RoleSystem and that req.Messages[0].Content does NOT contain "run-2" (and does NOT contain "<recent_gateway_events>"); this ensures TestAPI_EnqueueChatMessage_UpdatesDynamicSystemContext, api.EnqueueChatMessage, reqCh and waitForRequest cover the no-context follow-up case and prevent stale dynamic context regressions.internal/service/slack/recent_events.go (1)
143-154: 💤 Low valueConsider clarifying truncation semantics.
The
maxRunesparameter suggests a strict limit, but when truncation occurs the function returnsmaxRunes + 3runes (to accommodate"..."). For the large limits used (80–240), this is acceptable, but consider either:
- Renaming to
maxRunesBeforeEllipsisfor clarity, or- Subtracting 3 from maxRunes before slicing to enforce a strict post-ellipsis limit.
🤖 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 `@internal/service/slack/recent_events.go` around lines 143 - 154, sanitizeRecentGatewayField currently treats maxRunes as the number of runes to slice before adding "..." which makes the returned string length maxRunes+3; update the implementation or signature to clarify semantics: either rename the parameter to maxRunesBeforeEllipsis (and update all call sites to match) to document that the ellipsis is appended after slicing, or enforce a strict post-ellipsis limit by subtracting 3 from maxRunes before creating the slice in sanitizeRecentGatewayField so the final returned rune count never exceeds maxRunes; reference the function sanitizeRecentGatewayField and parameter maxRunes 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 `@internal/agent/session.go`:
- Around line 287-297: The SetDynamicSystemContext method currently returns
early when fn is nil, preventing callers from clearing a previously set
provider; change it to accept nil to clear sm.dynamicSystemCtx and propagate the
nil to the active loop by setting sm.dynamicSystemCtx = fn (even if fn == nil)
under the mutex and then calling loop.SetDynamicSystemContext(fn) if loop != nil
so future GetDynamicSystemContext(ctx) calls will observe the cleared provider;
update SessionManager.SetDynamicSystemContext and ensure
loop.SetDynamicSystemContext is invoked with nil to remove stale per-request
context.
---
Nitpick comments:
In `@internal/agent/api_test.go`:
- Around line 700-733: Add a second enqueue using plain context.Background()
after the existing run-2 test to verify dynamic system context is per-request:
call api.EnqueueChatMessage(context.Background(), sessionID, user,
ChatRequest{Message: "もう一度"}), wait for the next provider request via
waitForRequest(reqCh,...), then assert req.Messages[0].Role is llm.RoleSystem
and that req.Messages[0].Content does NOT contain "run-2" (and does NOT contain
"<recent_gateway_events>"); this ensures
TestAPI_EnqueueChatMessage_UpdatesDynamicSystemContext, api.EnqueueChatMessage,
reqCh and waitForRequest cover the no-context follow-up case and prevent stale
dynamic context regressions.
In `@internal/service/slack/recent_events.go`:
- Around line 143-154: sanitizeRecentGatewayField currently treats maxRunes as
the number of runes to slice before adding "..." which makes the returned string
length maxRunes+3; update the implementation or signature to clarify semantics:
either rename the parameter to maxRunesBeforeEllipsis (and update all call sites
to match) to document that the ellipsis is appended after slicing, or enforce a
strict post-ellipsis limit by subtracting 3 from maxRunes before creating the
slice in sanitizeRecentGatewayField so the final returned rune count never
exceeds maxRunes; reference the function sanitizeRecentGatewayField and
parameter maxRunes when making the change.
🪄 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: 15612f61-276c-440a-8014-deca031ff2f4
📒 Files selected for processing (10)
internal/agent/api.gointernal/agent/api_test.gointernal/agent/contextkeys.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/agent/session.gointernal/service/slack/bot.gointernal/service/slack/bot_test.gointernal/service/slack/monitor.gointernal/service/slack/recent_events.go
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/slack/bot_test.go (1)
434-434: 💤 Low valueConsider a more explicit ordering check.
The current approach using
strings.Indexassumes both strings are present (which is verified by earlier assertions) but is slightly fragile if the output format changes. Consider using a more structured approach or adding explicit index checks.💡 Example alternative approach
- assert.Less(t, strings.Index(text, "dag: dag-5"), strings.Index(text, "dag: dag-4")) + idx5 := strings.Index(text, "dag: dag-5") + idx4 := strings.Index(text, "dag: dag-4") + require.GreaterOrEqual(t, idx5, 0, "dag-5 should be present") + require.GreaterOrEqual(t, idx4, 0, "dag-4 should be present") + assert.Less(t, idx5, idx4, "dag-5 should appear before dag-4")🤖 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 `@internal/service/slack/bot_test.go` at line 434, The assertion using assert.Less(strings.Index(...)) is fragile; update the test in bot_test.go to explicitly verify presence and ordering by first computing idx5 := strings.Index(text, "dag: dag-5") and idx4 := strings.Index(text, "dag: dag-4"), assert both idx5 and idx4 are >= 0 (presence) and then assert idx5 < idx4 (ordering) or alternatively split text into lines and assert the line index of "dag-5" comes before "dag-4"; use the existing assert functions (assert.GreaterOrEqual/assert.Less) around idx5 and idx4 to make the checks explicit and robust.
🤖 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 `@internal/service/slack/bot_test.go`:
- Line 434: The assertion using assert.Less(strings.Index(...)) is fragile;
update the test in bot_test.go to explicitly verify presence and ordering by
first computing idx5 := strings.Index(text, "dag: dag-5") and idx4 :=
strings.Index(text, "dag: dag-4"), assert both idx5 and idx4 are >= 0 (presence)
and then assert idx5 < idx4 (ordering) or alternatively split text into lines
and assert the line index of "dag-5" comes before "dag-4"; use the existing
assert functions (assert.GreaterOrEqual/assert.Less) around idx5 and idx4 to
make the checks explicit and robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 146c104e-a35e-4015-9081-5a4d65fbbaac
📒 Files selected for processing (4)
internal/agent/api_test.gointernal/agent/session.gointernal/service/slack/bot_test.gointernal/service/slack/recent_events.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/agent/session.go
- internal/agent/api_test.go
- internal/service/slack/recent_events.go
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Summary
Changes
Related Issues
None.
Testing
Checklist
Summary by CodeRabbit
New Features
Tests