fix(core): resolve stream options when a notification wakes an idle thread#18640
fix(core): resolve stream options when a notification wakes an idle thread#18640CalebBarnes wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 35f21ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds notification stream-option hooks to ChangesNotification wake stream options dispatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR triageLinked issue check skipped for core contributor @CalebBarnes. PR complexity score
Applied label: Changed test gateChanged tests passed against the base branch; they should fail before the PR code is applied. Label: |
![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
🤖 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 @.changeset/notification-low-priority-persist.md:
- Line 5: Rewrite the changeset summary to focus on the user-visible outcome and
the bug fix: describe that low-priority notifications are now safely delayed
instead of triggering a failed processing attempt that could cause a “No model
selected” error. Remove internal implementation terms from the description, and
keep the wording outcome-focused and non-technical while still noting that
higher-priority notifications continue to be handled immediately.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7b35c35-87b2-40c5-9527-e5437dc3fb61
📒 Files selected for processing (3)
.changeset/notification-low-priority-persist.mdpackages/core/src/notifications/dispatcher.tspackages/core/src/notifications/notifications.test.ts
| '@mastra/core': patch | ||
| --- | ||
|
|
||
| Persist low-priority notifications instead of waking idle threads. The notification dispatcher's individual-record path defaulted to the `wake` idle behavior, which starts a run that requires a model/requestContext the dispatcher does not carry — surfacing as a "No model selected" error. Low-priority records now use `ifIdle: { behavior: 'persist' }`, mirroring the existing low-priority summary path, while higher-priority records keep the default wake behavior. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the changeset outcome-focused.
Line 5 reads like an implementation note. Please rewrite it around the behavior change and the failure it fixes, instead of internal terms like wake, ifIdle, and requestContext.
Suggested rewrite
-Persist low-priority notifications instead of waking idle threads. The notification dispatcher's individual-record path defaulted to the `wake` idle behavior, which starts a run that requires a model/requestContext the dispatcher does not carry — surfacing as a "No model selected" error. Low-priority records now use `ifIdle: { behavior: 'persist' }`, mirroring the existing low-priority summary path, while higher-priority records keep the default wake behavior.
+Fixed low-priority notifications failing on idle threads.
+
+Low-priority notifications now stay queued when a thread is idle instead of waking the thread and failing with "No model selected." Higher-priority notifications still wake the thread immediately.As per path instructions, changeset descriptions should "Avoid technical jargon" and "Highlight outcomes! What does change for the end user? Do not focus on internal implementation details."
potential_issue
📝 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.
| Persist low-priority notifications instead of waking idle threads. The notification dispatcher's individual-record path defaulted to the `wake` idle behavior, which starts a run that requires a model/requestContext the dispatcher does not carry — surfacing as a "No model selected" error. Low-priority records now use `ifIdle: { behavior: 'persist' }`, mirroring the existing low-priority summary path, while higher-priority records keep the default wake behavior. | |
| Fixed low-priority notifications failing on idle threads. | |
| Low-priority notifications now stay queued when a thread is idle instead of waking the thread and failing with "No model selected." Higher-priority notifications still wake the thread immediately. |
🤖 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 @.changeset/notification-low-priority-persist.md at line 5, Rewrite the
changeset summary to focus on the user-visible outcome and the bug fix: describe
that low-priority notifications are now safely delayed instead of triggering a
failed processing attempt that could cause a “No model selected” error. Remove
internal implementation terms from the description, and keep the wording
outcome-focused and non-technical while still noting that higher-priority
notifications continue to be handled immediately.
Source: Path instructions
dane-ai-mastra
Bot
added
the
tests: green ✅
CalebBarnes
force-pushed
the
fix/notification-low-priority-persist
branch
from
219a148 to
977b08c
Compare
CalebBarnes
changed the title
CalebBarnes
force-pushed
the
fix/notification-low-priority-persist
branch
from
977b08c to
2444cb5
Compare
dane-ai-mastra
Bot
added
tests: failing ❌
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 `@mastracode/src/index.ts`:
- Around line 485-515: The wake-time controller context stored in requestContext
is not JSON-safe because AgentControllerRequestContext includes closures and
live objects like getState, setState, session.state.update, and workspace.
Update the streamOptions/requestContext construction in index.ts to send only
serializable identifiers and plain data, and move rehydration of
controller/session methods to the receiving side that consumes the controller
context. Make sure the later agentControllerContext access in this flow no
longer depends on serialized function handles, and keep the UnixSocketPubSub
boundary limited to codec-safe values only.
In `@packages/core/src/notifications/dispatcher.ts`:
- Around line 134-146: The current `dispatcher.ts` target selection always falls
back to wake/default behavior and ignores low-priority individual records.
Update the `target` construction in the notification dispatch flow so records
marked low-priority use persistence instead of `ifIdle.behavior: 'wake'`, while
keeping the existing `agent.getNotificationStreamOptions` resolution for
wakeable cases. Use the existing `current` record fields and
`SendAgentSignalOptions` assembly to route low-priority notifications to persist
and only attach `streamOptions` when waking is intended.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd6e2776-1345-4591-87a5-3d57ad0b1548
📒 Files selected for processing (7)
.changeset/notification-wake-stream-options.mdmastracode/src/index.tspackages/core/src/agent/agent.tspackages/core/src/notifications/dispatcher.tspackages/core/src/notifications/notifications.test.tspackages/core/src/signals/signal-provider.tssignals/github/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/notification-wake-stream-options.md
| const requestContext = new RequestContext(); | ||
| const agentControllerContext: AgentControllerRequestContext = { | ||
| controllerId: controller.id, | ||
| state: session.state.get(), | ||
| getState: () => session.state.get(), | ||
| setState: updates => session.state.set(updates), | ||
| threadId, | ||
| resourceId, | ||
| session: { | ||
| id: session.identity.getId(), | ||
| ownerId: session.identity.getOwnerId(), | ||
| modeId, | ||
| modelId, | ||
| state: { | ||
| get: () => session.state.get(), | ||
| set: updates => session.state.set(updates), | ||
| update: updater => session.state.update(updater), | ||
| }, | ||
| }, | ||
| workspace: controller.getWorkspace(), | ||
| getSubagentModelId: params => session.subagents.model.get(params ?? {}), | ||
| }; | ||
| requestContext.set('controller', agentControllerContext); | ||
|
|
||
| return { | ||
| memory: { thread: threadId, resource: resourceId }, | ||
| requestContext, | ||
| maxSteps: 1000, | ||
| savePerStep: false, | ||
| requireToolApproval: (session.state.get() as Record<string, unknown>).yolo !== true, | ||
| modelSettings: { temperature: 1 }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep wake requestContext JSON-safe.
Line 507 stores an AgentControllerRequestContext with live closures (getState, setState, session.state.update) and workspace inside the wake streamOptions. That works in-process, but the UnixSocketPubSub path cannot carry those handles. After serialization, the controller entry is reduced to plain data, so the later agentControllerContext?.getState() call in this file will fail on notification-driven wakes.
Please send only serializable identifiers here and rehydrate the live controller/session methods on the receiving side instead. As per coding guidelines, "Rely on the codec at the UnixSocketPubSub boundary to handle Date, Error, Map, Set, and GeneratedFile; do not pass live handles or closures there."
🤖 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 `@mastracode/src/index.ts` around lines 485 - 515, The wake-time controller
context stored in requestContext is not JSON-safe because
AgentControllerRequestContext includes closures and live objects like getState,
setState, session.state.update, and workspace. Update the
streamOptions/requestContext construction in index.ts to send only serializable
identifiers and plain data, and move rehydration of controller/session methods
to the receiving side that consumes the controller context. Make sure the later
agentControllerContext access in this flow no longer depends on serialized
function handles, and keep the UnixSocketPubSub boundary limited to codec-safe
values only.
Source: Coding guidelines
| // A wake starts a fresh run, but the dispatcher carries no request context or | ||
| // model selection, which surfaces as "No model selected". Resolve the stream | ||
| // options from the signal provider that produced this record (matched by | ||
| // `source`) and attach them so the woken run has the context it needs. | ||
| // Resolved here because stream options carry live, non-serializable state | ||
| // (e.g. a RequestContext) that cannot be persisted on the record. | ||
| const streamOptions = await agent.getNotificationStreamOptions?.(current.source, { | ||
| resourceId: current.resourceId, | ||
| threadId: current.threadId, | ||
| }); | ||
| const target: SendAgentSignalOptions = streamOptions | ||
| ? { resourceId: current.resourceId, threadId: current.threadId, ifIdle: { behavior: 'wake', streamOptions } } | ||
| : { resourceId: current.resourceId, threadId: current.threadId }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve persist for low-priority individual records.
Line 140 resolves wake stream options for every record, and Line 144 falls back to default target behavior for low-priority records. That misses the PR’s intended fix: low-priority individual notifications should persist instead of waking idle runs.
Proposed fix
- const streamOptions = await agent.getNotificationStreamOptions?.(current.source, {
- resourceId: current.resourceId,
- threadId: current.threadId,
- });
- const target: SendAgentSignalOptions = streamOptions
- ? { resourceId: current.resourceId, threadId: current.threadId, ifIdle: { behavior: 'wake', streamOptions } }
- : { resourceId: current.resourceId, threadId: current.threadId };
+ const baseTarget = { resourceId: current.resourceId, threadId: current.threadId };
+ let target: SendAgentSignalOptions;
+ if (current.priority === 'low') {
+ target = { ...baseTarget, ifIdle: { behavior: 'persist' } };
+ } else {
+ const streamOptions = await agent.getNotificationStreamOptions?.(current.source, baseTarget);
+ target = streamOptions ? { ...baseTarget, ifIdle: { behavior: 'wake', streamOptions } } : baseTarget;
+ }📝 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.
| // A wake starts a fresh run, but the dispatcher carries no request context or | |
| // model selection, which surfaces as "No model selected". Resolve the stream | |
| // options from the signal provider that produced this record (matched by | |
| // `source`) and attach them so the woken run has the context it needs. | |
| // Resolved here because stream options carry live, non-serializable state | |
| // (e.g. a RequestContext) that cannot be persisted on the record. | |
| const streamOptions = await agent.getNotificationStreamOptions?.(current.source, { | |
| resourceId: current.resourceId, | |
| threadId: current.threadId, | |
| }); | |
| const target: SendAgentSignalOptions = streamOptions | |
| ? { resourceId: current.resourceId, threadId: current.threadId, ifIdle: { behavior: 'wake', streamOptions } } | |
| : { resourceId: current.resourceId, threadId: current.threadId }; | |
| // A wake starts a fresh run, but the dispatcher carries no request context or | |
| // model selection, which surfaces as "No model selected". Resolve the stream | |
| // options from the signal provider that produced this record (matched by | |
| // `source`) and attach them so the woken run has the context it needs. | |
| // Resolved here because stream options carry live, non-serializable state | |
| // (e.g. a RequestContext) that cannot be persisted on the record. | |
| const baseTarget = { resourceId: current.resourceId, threadId: current.threadId }; | |
| let target: SendAgentSignalOptions; | |
| if (current.priority === 'low') { | |
| target = { ...baseTarget, ifIdle: { behavior: 'persist' } }; | |
| } else { | |
| const streamOptions = await agent.getNotificationStreamOptions?.(current.source, baseTarget); | |
| target = streamOptions ? { ...baseTarget, ifIdle: { behavior: 'wake', streamOptions } } : baseTarget; | |
| } |
🤖 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 `@packages/core/src/notifications/dispatcher.ts` around lines 134 - 146, The
current `dispatcher.ts` target selection always falls back to wake/default
behavior and ignores low-priority individual records. Update the `target`
construction in the notification dispatch flow so records marked low-priority
use persistence instead of `ifIdle.behavior: 'wake'`, while keeping the existing
`agent.getNotificationStreamOptions` resolution for wakeable cases. Use the
existing `current` record fields and `SendAgentSignalOptions` assembly to route
low-priority notifications to persist and only attach `streamOptions` when
waking is intended.
2444cb5 to
374783b
Compare
dane-ai-mastra
Bot
added
tests: green ✅
CalebBarnes
force-pushed
the
fix/notification-low-priority-persist
branch
from
374783b to
9d03640
Compare
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 `@signals/github/src/index.ts`:
- Around line 1132-1134: The deprecated addAgent(agent, options) path updates
`#agentOptions` but does not keep the notification stream resolver in sync, unlike
the constructor path in the index.ts class. Update addAgent to also call
setNotificationStreamOptionsResolver whenever
options.getNotificationStreamOptions is provided so deferred GitHub
notifications wake with streamOptions and avoid the “No model selected” failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ded2f740-9789-4d49-8ab9-6ca61086639d
📒 Files selected for processing (6)
.changeset/notification-wake-stream-options.mdpackages/core/src/agent/agent.tspackages/core/src/notifications/dispatcher.tspackages/core/src/notifications/notifications.test.tspackages/core/src/signals/signal-provider.tssignals/github/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/notification-wake-stream-options.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/agent/agent.ts
- packages/core/src/notifications/notifications.test.ts
- packages/core/src/signals/signal-provider.ts
- packages/core/src/notifications/dispatcher.ts
| if (options.getNotificationStreamOptions) { | ||
| this.#agentOptions = { getNotificationStreamOptions: options.getNotificationStreamOptions }; | ||
| this.setNotificationStreamOptionsResolver(options.getNotificationStreamOptions); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the resolver in sync for the deprecated addAgent(...) path.
This only installs the wake-time resolver when it comes from the constructor. addAgent(agent, options) below still updates #agentOptions without calling setNotificationStreamOptionsResolver(...), so deferred GitHub notifications configured through the backward-compatible API will still wake idle threads without streamOptions and can hit the same “No model selected” failure again.
Suggested fix
constructor(options: GithubSignalsOptions = {}) {
super();
this.#options = options;
this.#syncClient = options.syncClient ?? new GitcrawlSyncClient({ command: options.gitcrawlCommand });
this.#repositoryResolver = options.repositoryResolver ?? new GitRemoteRepositoryResolver();
if (options.getNotificationStreamOptions) {
- this.#agentOptions = { getNotificationStreamOptions: options.getNotificationStreamOptions };
- this.setNotificationStreamOptionsResolver(options.getNotificationStreamOptions);
+ this.#agentOptions = { getNotificationStreamOptions: options.getNotificationStreamOptions };
+ this.setNotificationStreamOptionsResolver(options.getNotificationStreamOptions);
}
}
@@
addAgent(agent: GithubSignalAgent, options: GithubSignalAgentOptions = {}): void {
this.#agent = agent;
this.#agentOptions = options;
+ this.setNotificationStreamOptionsResolver(options.getNotificationStreamOptions);
}🤖 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 `@signals/github/src/index.ts` around lines 1132 - 1134, The deprecated
addAgent(agent, options) path updates `#agentOptions` but does not keep the
notification stream resolver in sync, unlike the constructor path in the
index.ts class. Update addAgent to also call
setNotificationStreamOptionsResolver whenever
options.getNotificationStreamOptions is provided so deferred GitHub
notifications wake with streamOptions and avoid the “No model selected” failure.
…hread A deferred notification that wakes an idle thread starts a fresh run, but the dispatcher carries no request context or model selection. That run had no model, surfacing as "No model selected". Signal providers can now implement getNotificationStreamOptions(target), routed by a notificationSource getter that defaults to the provider id. The dispatcher matches a due record back to the provider that produced it (record.source === provider.notificationSource) and resolves the stream options at send time, attaching them to the wake target's ifIdle.streamOptions so the woken run has the request context and model it needs. Resolved at dispatch rather than persisted because stream options carry live, non-serializable state (e.g. a RequestContext) that cannot be stored on the record. GithubSignals implements the resolver from its existing session->model logic; mastracode wires that single resolver into the provider with no agent-config duplication. Co-Authored-By: Mastra Code (anthropic/claude-opus-4-8) <noreply@mastra.ai>
CalebBarnes
force-pushed
the
fix/notification-low-priority-persist
branch
from
9d03640 to
35f21ed
Compare
dane-ai-mastra
Bot
added
tests: failing ❌
dane-ai-mastra
Bot
added
tests: green ✅
1 participant
What
Fixes the "No model selected" error that occurred when a deferred notification woke an idle thread.
When a notification is due and its target thread is idle, the dispatcher starts a fresh run to deliver it. That run carried no request context or model selection, so it had no model to run with — surfacing as
No model selected.How
Signal providers can now resolve the stream options a woken run needs:
SignalProvidergains an optionalgetNotificationStreamOptions(target)method and anotificationSourcegetter (defaults to the providerid).record.source === provider.notificationSource) and calls that provider's resolver at send time, attaching the result to the wake target'sifIdle.streamOptions.GithubSignalsimplements the resolver using its existing session → model/requestContext logic, and overridesnotificationSourceto'github'(its records carrysource: 'github'while itsidis'github-signals').Resolved at dispatch time (not persisted on the record) because stream options carry live, non-serializable state such as a
RequestContextand closures.Why not persist / change defaults
Stream options can't be serialized to storage, so the resolution has to happen in-process at dispatch. The fix makes wake actually work rather than avoiding waking — no framework defaults, agent config, or summary behavior changed.
Tests
getNotificationStreamOptions, the wake target carriesifIdle: { behavior: 'wake', streamOptions }with the resolved options, and the resolver is called with(source, { resourceId, threadId }).Verification
@mastra/coretype-check clean; 59 notification tests pass.@mastra/github-signalstype-check clean; 58 tests pass.ELI5
Sometimes a notification wakes an idle worker, but the worker doesn’t know which “model/context” to use, so it errors. This PR teaches the system to figure out the right wake-up details at the moment of dispatch, so the worker can start correctly.
What changed
notificationSourceand an optionalgetNotificationStreamOptions(target)hook to signal providers.streamOptionsat dispatch time (instead of relying on persisted data).ifIdle.streamOptions) so the woken run receives the resolved request context/model selection when available.GithubSignalsby wiring it into the existing session-to-model/requestContext logic and settingnotificationSourcetogithub.getNotificationStreamOptionsis called with the expected source/target identifiers and that wake delivery attaches the resolvedstreamOptions.Result
Deferred notifications that wake idle threads now include the needed, correctly-resolved execution options—preventing the “No model selected” failure—while keeping default wake behavior covered.