fix(schedules): submit scheduled runs via prompt_async with SSE monitoring - #335
fix(schedules): submit scheduled runs via prompt_async with SSE monitoring#335chriswritescode-dev wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughScheduled runs now submit prompts through ChangesScheduled execution monitoring
Scheduled question permissions
Repository ignore configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduleService
participant OpenCodeAPI
participant SessionMonitor
participant SSEAggregator
ScheduleService->>OpenCodeAPI: POST /session/{id}/prompt_async
ScheduleService->>SessionMonitor: markSubmitted
OpenCodeAPI-->>SessionMonitor: status and session.idle events
SessionMonitor-->>ScheduleService: completion signal
ScheduleService->>SSEAggregator: provide active session reference
SSEAggregator-->>SSEAggregator: replay status by directory
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 4
🧹 Nitpick comments (3)
backend/test/services/schedules.test.ts (1)
384-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
stoppedoutcome.The new tests cover
busyandsettled. They do not cover the case where the session reports idle and no assistant message is completed and no error is set. That path returnsSESSION_STOPPED_ERRORand marks the runfailed. It is the branch that the linked issue reports in production.Add a test that emits
session.idlewhile/session/:id/messagereturns no completed assistant message, then assertstatus: 'failed'with the stopped error text.As per coding guidelines: "Maintain at least 80% test coverage for the backend."
🤖 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 `@backend/test/services/schedules.test.ts` around lines 384 - 446, Add a ScheduleService test covering the stopped outcome: configure the session to report idle while /session/:id/message returns no completed assistant message and no error, emit the matching session.idle event, then assert updateScheduleRun marks the run failed with the SESSION_STOPPED_ERROR text. Keep the existing busy and settled scenarios unchanged.Source: Coding guidelines
backend/src/services/schedules.ts (1)
311-365: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
nextSignalagainst concurrent consumers.
waitingholds a single resolver. If two callers awaitnextSignal()at the same time, the first resolver is overwritten and its promise never settles. Only one consumer exists today, so this is a defensive hardening item, not a current defect.♻️ Suggested change to queue waiters
- const queued: SessionSignal[] = [] - let waiting: ((signal: SessionSignal) => void) | null = null + const queued: SessionSignal[] = [] + const waiters: ((signal: SessionSignal) => void)[] = [] let disposed = false const push = (signal: SessionSignal): void => { - if (waiting) { - const resolve = waiting - waiting = null - resolve(signal) - return - } + const resolve = waiters.shift() + if (resolve) { + resolve(signal) + return + } queued.push(signal) }- return new Promise<SessionSignal>((resolve) => { waiting = resolve }) + return new Promise<SessionSignal>((resolve) => { waiters.push(resolve) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/schedules.ts` around lines 311 - 365, Update the session signal consumer returned by the surrounding factory so nextSignal prevents concurrent pending consumers instead of overwriting the single waiting resolver. Preserve queued-signal delivery, disposed handling, and existing behavior for the supported single-consumer flow, while ensuring any second simultaneous call is handled explicitly and no promise remains unsettled.shared/src/schemas/schedule.ts (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the added TypeScript comments in both files.
The repository requires TypeScript and TSX code to remain self-documenting.
shared/src/schemas/schedule.ts#L62-L64: remove the added JSDoc text or move it to external documentation.frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx#L102-L105: remove the added block comment.As per coding guidelines:
**/*.{ts,tsx,js,jsx}: Do not add code comments; code must be self-documenting.🤖 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 `@shared/src/schemas/schedule.ts` around lines 62 - 64, Remove the added TypeScript comments from shared/src/schemas/schedule.ts lines 62-64 and the block comment from frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx lines 102-105, leaving the surrounding code unchanged.Source: Coding guidelines
🤖 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 `@backend/src/services/schedules.ts`:
- Around line 1124-1128: Remove the added block comment near the assistant
outcome handling. Preserve its intent through a self-documenting method name
such as readOutcomeOnlyWhenSessionIdle, or retain readAssistantOutcome while
relying on the explicit AssistantOutcome union.
- Around line 1153-1181: Bound waitForAssistantMessage so it cannot remain
blocked indefinitely when sessionMonitor.nextSignal() never emits another event.
Add a periodic timeout or re-check that invokes readAssistantOutcome and
preserves the existing busy, settled, and stopped result handling, ensuring the
wait eventually returns an error or terminal outcome and allows cleanup to
remove the run from activeRuns.
In `@backend/src/services/sse-aggregator.ts`:
- Around line 539-545: Update getScheduledSessions to wrap the
scheduledSessionsResolver call in try/catch, log the resolver failure using the
service’s existing logging mechanism, and return an empty ScheduledSessionRef
array on error so replaySessionStatusesForTrackedDirectories continues
processing all directories.
In `@frontend/src/components/schedules/GeneralTab.tsx`:
- Around line 185-193: Give the Radix Switch in GeneralTab the accessible name
“Allow questions” by associating it with the visible label via a stable id and
aria-labelledby, or an equivalent aria-label. Update the related permissions
test to locate it with getByRole('switch', { name: 'Allow questions' }) instead
of relying on switch indexes.
---
Nitpick comments:
In `@backend/src/services/schedules.ts`:
- Around line 311-365: Update the session signal consumer returned by the
surrounding factory so nextSignal prevents concurrent pending consumers instead
of overwriting the single waiting resolver. Preserve queued-signal delivery,
disposed handling, and existing behavior for the supported single-consumer flow,
while ensuring any second simultaneous call is handled explicitly and no promise
remains unsettled.
In `@backend/test/services/schedules.test.ts`:
- Around line 384-446: Add a ScheduleService test covering the stopped outcome:
configure the session to report idle while /session/:id/message returns no
completed assistant message and no error, emit the matching session.idle event,
then assert updateScheduleRun marks the run failed with the
SESSION_STOPPED_ERROR text. Keep the existing busy and settled scenarios
unchanged.
In `@shared/src/schemas/schedule.ts`:
- Around line 62-64: Remove the added TypeScript comments from
shared/src/schemas/schedule.ts lines 62-64 and the block comment from
frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx lines
102-105, leaving the surrounding code unchanged.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cff9beaf-9692-4122-8c97-d1731b00dcfd
📒 Files selected for processing (13)
.gitignorebackend/src/index.tsbackend/src/services/schedules.tsbackend/src/services/sse-aggregator.tsbackend/test/db/schedules.permission.test.tsbackend/test/services/schedule-permissions.test.tsbackend/test/services/schedules.permission.test.tsbackend/test/services/schedules.test.tsbackend/test/services/sse-aggregator.test.tsfrontend/src/components/schedules/GeneralTab.tsxfrontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsxfrontend/src/components/schedules/ScheduleJobDialog.tsxshared/src/schemas/schedule.ts
| /** | ||
| * An assistant message completing is not the end of a run: a multi-step agent | ||
| * settles one message per tool call. Only a session that has gone idle has | ||
| * finished, and only then is the final message guaranteed to carry its parts. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the added block comment.
The coding guidelines forbid comments in TypeScript files. Encode the intent in the method name instead, for example readOutcomeOnlyWhenSessionIdle, or keep readAssistantOutcome and rely on the explicit AssistantOutcome union.
As per coding guidelines: "Do not add comments; code should be self-documenting."
🤖 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 `@backend/src/services/schedules.ts` around lines 1124 - 1128, Remove the added
block comment near the assistant outcome handling. Preserve its intent through a
self-documenting method name such as readOutcomeOnlyWhenSessionIdle, or retain
readAssistantOutcome while relying on the explicit AssistantOutcome union.
Source: Coding guidelines
| private async waitForAssistantMessage( | ||
| job: ScheduleJob, | ||
| sessionId: string, | ||
| sessionMonitor: SessionMonitor, | ||
| directory: string, | ||
| ): Promise<{ responseText: string | null; errorText: string | null }> { | ||
| const startedAt = Date.now() | ||
|
|
||
| while (Date.now() - startedAt < RUN_POLL_TIMEOUT_MS) { | ||
| const messages = await this.listSessionMessages(directory, sessionId) | ||
| const assistantState = getAssistantMessageState(messages) | ||
| for (;;) { | ||
| const signal = await sessionMonitor.nextSignal() | ||
|
|
||
| if (assistantState && (assistantState.completed || assistantState.errorText)) { | ||
| if (signal.errorText || signal.disposed) { | ||
| const messages = await this.listSessionMessages(directory, sessionId) | ||
| return { | ||
| responseText: assistantState.responseText, | ||
| errorText: assistantState.errorText, | ||
| responseText: getAssistantMessageState(messages)?.responseText ?? null, | ||
| errorText: signal.errorText ?? SESSION_STOPPED_ERROR, | ||
| } | ||
| } | ||
|
|
||
| const sessionErrorText = sessionMonitor.getErrorText() | ||
| if (sessionErrorText) { | ||
| return { | ||
| responseText: null, | ||
| errorText: sessionErrorText, | ||
| } | ||
| } | ||
| const outcome = await this.readAssistantOutcome(directory, sessionId) | ||
|
|
||
| if (sessionMonitor.isIdle()) { | ||
| return { | ||
| responseText: null, | ||
| errorText: 'The session became idle without producing an assistant response. Open the linked session to inspect any pending questions, permissions, or provider issues.', | ||
| } | ||
| if (outcome.kind === 'busy') { | ||
| continue | ||
| } | ||
|
|
||
| await Bun.sleep(RUN_POLL_INTERVAL_MS) | ||
| } | ||
| if (outcome.kind === 'settled') { | ||
| return { responseText: outcome.responseText, errorText: outcome.errorText } | ||
| } | ||
|
|
||
| return { | ||
| responseText: null, | ||
| errorText: 'Timed out waiting for the assistant response. Open the linked session to inspect any pending questions, permissions, or provider issues.', | ||
| return { responseText: outcome.responseText, errorText: SESSION_STOPPED_ERROR } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
waitForAssistantMessage can wait forever.
The previous implementation used fixed-interval polling with a timeout. The new loop blocks on sessionMonitor.nextSignal() and only resumes when a session.idle, session.status idle, or session.error event arrives for the directory, or when dispose() runs. dispose() only runs in the finally of monitorRunCompletion, which is reached after this loop returns, so it cannot unblock the loop.
If the upstream event for the session is never delivered, for example when the OpenCode process for that directory dies and the aggregator never emits a further status for the session, the run stays running forever. ScheduleService.activeRuns keeps the job id, so runJob then rejects every later trigger with "Schedule is already running".
Add a bounded fallback. One option is a maximum wait that re-reads the session state, another is a periodic re-check that calls readAssistantOutcome even without a signal.
🛡️ Sketch of a bounded wait
- for (;;) {
- const signal = await sessionMonitor.nextSignal()
+ for (;;) {
+ const signal = await Promise.race([
+ sessionMonitor.nextSignal(),
+ Bun.sleep(SESSION_SIGNAL_RECHECK_MS).then((): SessionSignal | null => null),
+ ])
+
+ if (!signal) {
+ const polled = await this.readAssistantOutcome(directory, sessionId)
+ if (polled.kind === 'busy') continue
+ if (polled.kind === 'settled') {
+ return { responseText: polled.responseText, errorText: polled.errorText }
+ }
+ return { responseText: polled.responseText, errorText: SESSION_STOPPED_ERROR }
+ }Confirm whether another component already bounds scheduled run duration.
#!/bin/bash
# Description: Look for any timeout, watchdog, or stale-run reaper covering schedule runs.
set -euo pipefail
rg -nP --type=ts -C4 '\b(setTimeout|AbortSignal\.timeout|TIMEOUT|_MS)\b' backend/src/services/schedules.ts backend/src/services/schedule-worktree.ts 2>/dev/null || true
fd -e ts . backend/src --exec rg -nP -C4 'stale|watchdog|reap|maxRunDuration|runTimeout' {} \;
rg -nP --type=ts -C4 '\bactiveRuns\b' backend/src🤖 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 `@backend/src/services/schedules.ts` around lines 1153 - 1181, Bound
waitForAssistantMessage so it cannot remain blocked indefinitely when
sessionMonitor.nextSignal() never emits another event. Add a periodic timeout or
re-check that invokes readAssistantOutcome and preserves the existing busy,
settled, and stopped result handling, ensuring the wait eventually returns an
error or terminal outcome and allows cleanup to remove the run from activeRuns.
| getScheduledSessionIds(): Set<string> { | ||
| return this.scheduledSessionsResolver?.() ?? new Set() | ||
| return new Set(this.getScheduledSessions().map(ref => ref.sessionID)) | ||
| } | ||
|
|
||
| private getScheduledSessions(): ScheduledSessionRef[] { | ||
| return this.scheduledSessionsResolver?.() ?? [] | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the resolver call against throwing.
replaySessionStatusesForTrackedDirectories runs from void this.replaySessionStatusesForTrackedDirectories() in es.onopen. It calls getScheduledSessionsByDirectory first, which calls the resolver. The resolver is scheduleService.getActiveRunSessions(), which reads SQLite. If that read throws, the rejection is unhandled and the whole status replay is skipped for every directory, including client directories.
Wrap the resolver call in try/catch and log the failure.
🛡️ Proposed fix
private getScheduledSessions(): ScheduledSessionRef[] {
- return this.scheduledSessionsResolver?.() ?? []
+ try {
+ return this.scheduledSessionsResolver?.() ?? []
+ } catch (error) {
+ logger.warn(`replay: failed to resolve scheduled sessions: ${String(error)}`)
+ return []
+ }
}As per coding guidelines: "Handle errors appropriately with try/catch where required."
📝 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.
| getScheduledSessionIds(): Set<string> { | |
| return this.scheduledSessionsResolver?.() ?? new Set() | |
| return new Set(this.getScheduledSessions().map(ref => ref.sessionID)) | |
| } | |
| private getScheduledSessions(): ScheduledSessionRef[] { | |
| return this.scheduledSessionsResolver?.() ?? [] | |
| } | |
| getScheduledSessionIds(): Set<string> { | |
| return new Set(this.getScheduledSessions().map(ref => ref.sessionID)) | |
| } | |
| private getScheduledSessions(): ScheduledSessionRef[] { | |
| try { | |
| return this.scheduledSessionsResolver?.() ?? [] | |
| } catch (error) { | |
| logger.warn(`replay: failed to resolve scheduled sessions: ${String(error)}`) | |
| return [] | |
| } | |
| } |
🤖 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 `@backend/src/services/sse-aggregator.ts` around lines 539 - 545, Update
getScheduledSessions to wrap the scheduledSessionsResolver call in try/catch,
log the resolver failure using the service’s existing logging mechanism, and
return an empty ScheduledSessionRef array on error so
replaySessionStatusesForTrackedDirectories continues processing all directories.
Source: Coding guidelines
| <div className="rounded-lg border border-border bg-card p-4"> | ||
| <div className="flex items-center justify-between gap-4"> | ||
| <div className="flex items-center gap-2"> | ||
| <p className="text-sm font-medium">Allow questions</p> | ||
| <InfoHint text="Scheduled runs are unattended. When disabled, the agent's question tool is denied so a run can never stall waiting for an answer that nobody is there to give." /> | ||
| </div> | ||
| <Switch checked={allowQuestions} onCheckedChange={onAllowQuestionsChange} /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'GeneralTab\.tsx|ScheduleJobDialog.permissions.test.tsx' . || true
echo "== GeneralTab outline =="
if [ -f frontend/src/components/schedules/GeneralTab.tsx ]; then
wc -l frontend/src/components/schedules/GeneralTab.tsx
ast-grep outline frontend/src/components/schedules/GeneralTab.tsx || true
sed -n '150,210p' frontend/src/components/schedules/GeneralTab.tsx
fi
echo "== search Switch imports and occurrences in GeneralTab =="
rg -n "import .*Switch|Switch|Allow questions|allowQuestions|onAllowQuestionsChange" frontend/src/components/schedules/GeneralTab.tsx || true
echo "== test relevant content =="
if [ -f frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx ]; then
wc -l frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx
rg -n "Allow questions|getByRole|getByLabelText|queryByRole|allow.questions|allow questions|question" frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx || true
fiRepository: chriswritescode-dev/opencode-manager
Length of output: 5352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== components/ui/switch =="
fd -a 'switch\.(tsx|ts|jsx|js)$' frontend/src/components/ui || true
if [ -f frontend/src/components/ui/switch.tsx ]; then
wc -l frontend/src/components/ui/switch.tsx
sed -n '1,220p' frontend/src/components/ui/switch.tsx
fi
echo "== relevant tests =="
sed -n '180,245p' frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx
echo "== all allowQuestions switch usages =="
rg -n "getByRole\('switch'|getByRole\(\"switch\"|getByLabel|Allow questions|allowQuestions" frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx frontend/src/components/schedules/GeneralTab.tsx || true
echo "== React switch accessibility pattern probe from source text =="
python3 - <<'PY'
from pathlib import Path
p = Path('frontend/src/components/ui/switch.tsx')
def load():
try:
return p.read_text()
except Exception as e:
print(f"missing or unreadable {p}: {e}")
return ""
s=load()
print("contains aria-labelledby:", "aria-labelledby" in s)
print("uses SwitchPrimitive.Root:", "SwitchPrimitive.Root" in s or "Switch.Root" in s)
print("role=switch:", "role=\"switch\"" in s or "role='switch'" in s)
print("tabIndex set:", "tabIndex" in s)
PYRepository: chriswritescode-dev/opencode-manager
Length of output: 4817
Give the question switch an accessible name.
GeneralTab.tsx uses a Radix Switch, but the Allow questions label is only in a nearby <p>, so the switch can be exposed without a name. Add a stable id on the <p> and associate it with Switch via aria-labelledby, or add aria-label="Allow questions". Update frontend/src/components/schedules/ScheduleJobDialog.permissions.test.tsx to find the switch with getByRole('switch', { name: 'Allow questions' }) instead of index-based switch selection.
🤖 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 `@frontend/src/components/schedules/GeneralTab.tsx` around lines 185 - 193,
Give the Radix Switch in GeneralTab the accessible name “Allow questions” by
associating it with the visible label via a stable id and aria-labelledby, or an
equivalent aria-label. Update the related permissions test to locate it with
getByRole('switch', { name: 'Allow questions' }) instead of relying on switch
indexes.
1 participant
Scheduled runs submitted synchronously via
POST /session/:id/message, so a connection interruption or OpenCode restart mid-run aborted the job withProxy request failed. Submission now usesPOST /session/:id/prompt_asyncand completion is tracked through the existing SSE aggregator (session idle/error signals plus a settled assistant-message read), with directory-scoped session status replay covering scheduled sessions across upstream reconnects. Adds anallowQuestionspermission toggle (default off) so unattended runs deny agent questions instead of stalling. Fixes #334.Summary
Type of Change
Checklist
pnpm lintpasses locallypnpm typecheckpasses locallySummary by CodeRabbit
New Features
Bug Fixes
Tests