elias/oh-my-opencode
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address code review feedback on PR #1988

Browse Source 
- Fix operator precedence bug in hasActiveWork boolean expression
- Reuse getMainSessionStatus result from watchdog to avoid duplicate API calls
- Add flag to only check secondary timeout once to avoid unnecessary API traffic
This commit is contained in:
CloudWaddie 2026-02-20 14:45:54 +10:30 committed by YeonGyu-Kim
parent 159ade05cc
commit 54d0dcde48
1 changed files with 25 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
src/cli/run/poll-for-completion.ts
View File

@ -40,6 +40,7 @@ export async function pollForCompletion(
let consecutiveCompleteChecks = 0 let consecutiveCompleteChecks = 0
let errorCycleCount = 0 let errorCycleCount = 0
let firstWorkTimestamp: number | null = null let firstWorkTimestamp: number | null = null
let secondaryTimeoutChecked = false
const pollStartTimestamp = Date.now() const pollStartTimestamp = Date.now()
while (!abortController.signal.aborted) { while (!abortController.signal.aborted) {
@ -69,6 +70,7 @@ export async function pollForCompletion(
} }
// Watchdog: if no events received for N seconds, verify session status via API // Watchdog: if no events received for N seconds, verify session status via API
let mainSessionStatus: "idle" | "busy" | "retry" | null = null
if (eventState.lastEventTimestamp !== null) { if (eventState.lastEventTimestamp !== null) {
const timeSinceLastEvent = Date.now() - eventState.lastEventTimestamp const timeSinceLastEvent = Date.now() - eventState.lastEventTimestamp
if (timeSinceLastEvent > eventWatchdogMs) { if (timeSinceLastEvent > eventWatchdogMs) {
@ -82,10 +84,10 @@ export async function pollForCompletion(
) )
// Force check session status directly // Force check session status directly
const directStatus = await getMainSessionStatus(ctx) mainSessionStatus = await getMainSessionStatus(ctx)
if (directStatus === "idle") { if (mainSessionStatus === "idle") {
eventState.mainSessionIdle = true eventState.mainSessionIdle = true
} else if (directStatus === "busy" || directStatus === "retry") { } else if (mainSessionStatus === "busy" || mainSessionStatus === "retry") {
eventState.mainSessionIdle = false eventState.mainSessionIdle = false
} }
@ -94,7 +96,10 @@ export async function pollForCompletion(
} }
} }
const mainSessionStatus = await getMainSessionStatus(ctx) // Only call getMainSessionStatus if watchdog didn't already check
if (mainSessionStatus === null) {
mainSessionStatus = await getMainSessionStatus(ctx)
}
if (mainSessionStatus === "busy" || mainSessionStatus === "retry") { if (mainSessionStatus === "busy" || mainSessionStatus === "retry") {
eventState.mainSessionIdle = false eventState.mainSessionIdle = false
} else if (mainSessionStatus === "idle") { } else if (mainSessionStatus === "idle") {
@ -119,7 +124,12 @@ export async function pollForCompletion(
// Secondary timeout: if we've been polling for reasonable time but haven't // Secondary timeout: if we've been polling for reasonable time but haven't
// received meaningful work via events, check if there's active work via API // received meaningful work via events, check if there's active work via API
if (Date.now() - pollStartTimestamp > secondaryMeaningfulWorkTimeoutMs) { // Only check once to avoid unnecessary API calls every poll cycle
if (
Date.now() - pollStartTimestamp > secondaryMeaningfulWorkTimeoutMs &&
!secondaryTimeoutChecked
) {
secondaryTimeoutChecked = true
// Check if session actually has pending work (children, todos, etc.) // Check if session actually has pending work (children, todos, etc.)
const childrenRes = await ctx.client.session.children({ const childrenRes = await ctx.client.session.children({
path: { id: ctx.sessionID }, path: { id: ctx.sessionID },
@ -132,18 +142,16 @@ export async function pollForCompletion(
}) })
const todos = normalizeSDKResponse(todosRes, [] as unknown[]) const todos = normalizeSDKResponse(todosRes, [] as unknown[])
const hasActiveWork = const hasActiveChildren =
Array.isArray(children) Array.isArray(children) && children.length > 0
? children.length > 0 const hasActiveTodos =
: false || Array.isArray(todos) Array.isArray(todos) &&
? todos.some( todos.some(
( (t: unknown) =>
t: unknown (t as { status?: string })?.status !== "completed" &&
) => (t as { status?: string })?.status !== "cancelled"
(t as { status?: string })?.status !== "completed" && )
(t as { status?: string })?.status !== "cancelled" const hasActiveWork = hasActiveChildren || hasActiveTodos
)
: false
if (hasActiveWork) { if (hasActiveWork) {
// Assume meaningful work is happening even without events // Assume meaningful work is happening even without events