Skip to content

Commit 762ac7c

Browse files
sahrizviclaude
andcommitted
fix: [AI-7519] address 3 safe OpenCodeReview findings
Dev-punia bot flagged 6 items. Audited each; applying only the 3 that are legitimate improvements AND won't affect runtime behavior: - **pty-tui.ts waitForText — reset RegExp.lastIndex before test** (finding 3). Pure defensive fix. A RegExp with the `/g` flag has stateful `.test()` — repeat calls on the same string return false after the first match. Our current specs don't use `/g` so nothing changes today; future callers won't need to know. - **pty-tui.ts waitForText — fail fast on child exit** (finding 4). Poll loop now breaks when the child process exits with a distinct "child exit before match" error message. If the TUI crashes mid-test, we now surface it in seconds instead of burning the full 8s timeout. - **prompt.ts bootstrap span — capture endTime once** (finding 6). Two Date.now() calls straddling a clock tick would make duration_ms not match `endTime - startTime`. Trivial fix, cleaner math. Deferred (need investigation before applying): - Finding 1 (Promise.allSettled for concurrent phase publish) — legit perf tweak but caused an e2e regression in an initial attempt that I couldn't cleanly attribute. Worth revisiting once we understand the Layer/Effect concurrency semantics of runStatus called from a hot path. - Finding 2 (accumulate raw, strip on read) — legit chunk-boundary concern in theory, but the current per-chunk stripAnsi has been reliable in practice. Changing text() from O(1) memoized string to an O(n) computation is a semantics change worth its own review. - Finding 5 (step-aware resolve-tools span name) — legit telemetry hygiene and the ternary logic is correct on paper (step===1 fires before resolveTools), but re-running the e2e after the change didn't see "Discovering tools" render; couldn't confirm the label reliably survives on subsequent steps. Deferred for a focused pass. Local validation - Typecheck clean. - Session + fork-guards: 806 pass / 0 fail / 12 skip / 45 todo. - E2E ran 3 consecutive times, all pass, "Discovering tools" observed on every run. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2
1 parent 7bd4035 commit 762ac7c

2 files changed

Lines changed: 27 additions & 4 deletions

File tree

packages/opencode/src/session/prompt.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,13 +1173,16 @@ export namespace SessionPrompt {
11731173
// Companion sub-spans (bootstrap.session-get, bootstrap.config-get,
11741174
// bootstrap.resolve-tools, etc.) are already emitted; this span gives
11751175
// the waterfall a single top-level duration to render + gate against.
1176+
// Capture endTime once so duration_ms is guaranteed to match — two
1177+
// Date.now() calls can straddle a clock tick.
1178+
const bootstrapEnd = Date.now()
11761179
Tracer.active?.logSpan({
11771180
name: "bootstrap",
11781181
startTime: bootstrapStart,
1179-
endTime: Date.now(),
1182+
endTime: bootstrapEnd,
11801183
input: { agent: agent.name, sessionID },
11811184
output: {
1182-
duration_ms: Date.now() - bootstrapStart,
1185+
duration_ms: bootstrapEnd - bootstrapStart,
11831186
system_parts: system.length,
11841187
},
11851188
})

packages/opencode/test/fixture/pty-tui.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,12 @@ export async function launchTui(opts: LaunchOptions): Promise<TuiSession> {
146146
stripped += stripAnsi(chunk)
147147
})
148148

149+
let hasExited = false
149150
const exited = new Promise<{ exitCode: number; signal?: number | string }>((resolve) => {
150-
child.onExit((event) => resolve(event))
151+
child.onExit((event) => {
152+
hasExited = true
153+
resolve(event)
154+
})
151155
})
152156

153157
let disposed = false
@@ -177,11 +181,27 @@ export async function launchTui(opts: LaunchOptions): Promise<TuiSession> {
177181
async waitForText(needle, waitOpts) {
178182
const timeoutMs = waitOpts?.timeoutMs ?? DEFAULT_WAIT_TIMEOUT_MS
179183
const deadline = Date.now() + timeoutMs
180-
const matches = (s: string) => (typeof needle === "string" ? s.includes(needle) : needle.test(s))
184+
const matches = (s: string) => {
185+
if (typeof needle === "string") return s.includes(needle)
186+
// Reset lastIndex so a RegExp with the `g` flag (which makes .test()
187+
// stateful) still returns consistently across successive polls.
188+
needle.lastIndex = 0
189+
return needle.test(s)
190+
}
181191
if (matches(stripped)) return
182192
while (Date.now() < deadline) {
183193
await new Promise((r) => setTimeout(r, POLL_INTERVAL_MS))
184194
if (matches(stripped)) return
195+
// Fail fast if the child exited before the needle showed up —
196+
// otherwise a crash silently burns the full timeout budget and
197+
// masks the underlying cause.
198+
if (hasExited) {
199+
throw new Error(
200+
`waitForText saw child exit before matching ${
201+
typeof needle === "string" ? JSON.stringify(needle) : needle.toString()
202+
}\n\n--- captured (stripped, last 4000 chars) ---\n${stripped.slice(-4000)}\n--- end ---`,
203+
)
204+
}
185205
}
186206
throw new Error(
187207
`waitForText timed out after ${timeoutMs}ms waiting for ${

0 commit comments

Comments
 (0)