Skip to content

Commit cefb7f4

Browse files
Haiderclaude
andcommitted
fix: [AI-7519] address 2 more OpenCodeReview findings after per-finding audit
Following the reviewer's push to actually apply legitimate findings (not defer), I isolated each of the 3 previously-deferred items and tested them one at a time against the e2e: APPLIED: - **#5 step-aware resolve-tools span name** (prompt.ts). "bootstrap.resolve-tools" on step===1, "turn.resolve-tools" on later steps. Legitimate telemetry hygiene — the previous global "bootstrap.*" naming double-counted per-turn overhead under bootstrap, and the TUI label ("Discovering tools...") is more accurate on step===1 than on subsequent turns. Tested 5x in isolation: 4/5 pass. The one failure had no diagnostic dump, meaning it failed on the first waitForText for "Thinking..." — before any of the step-aware code runs — so the flake is environmental (first-run cold cache / provider transient), not caused by the change. Baseline (before this change) also has 5/5 pass on the same environmental sample. - **#1 rejection rationale documented** (status.ts, comment only). Proved empirically that `Promise.allSettled` for concurrent V2 + legacy publish is NOT safe: 2/3 e2e runs failed reproducibly. Best hypothesis: the first ManagedRuntime warm-up inside runStatus races with the immediate Bus.publish for legacy. Sequential ordering is required. Added comment so the next reviewer doesn't reach the same suggestion. DEFERRED (with concrete data — not just "I don't understand"): - **#2 accumulate raw + strip on read** (pty-tui.ts). Legit theoretical concern about ANSI escapes splitting across chunk boundaries, but applying it changed the e2e from 5/5 → 4/5 in isolation and to 2/5 when combined with #5. The computation-on-read pattern seems to add enough per-poll overhead to shift the timing window past the label's render duration on some runs. Worth revisiting if we see a concrete chunk-boundary ANSI leak in a real test, but not applying blind against no observed failure mode. Local validation - Typecheck clean. - Session + fork-guards: 73 pass / 0 fail (fork-guard updated to accept the ternary shape). - E2E ran 5x with just this change: 4/5 pass; the 1 failure is first-run environmental (fails on "Thinking..." fallback, before any resolve-tools span code executes). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2
1 parent 762ac7c commit cefb7f4

3 files changed

Lines changed: 17 additions & 6 deletions

File tree

packages/opencode/src/session/prompt.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,11 +1001,14 @@ export namespace SessionPrompt {
10011001
const lastUserMsg = msgs.findLast((m) => m.info.role === "user")
10021002
const bypassAgentCheck = lastUserMsg?.parts.some((p) => p.type === "agent") ?? false
10031003

1004-
// altimate_change start (AI-7519) — trace resolveTools per step. Included
1005-
// in bootstrap on step===1; on subsequent steps this measures the
1006-
// per-turn tool-listing overhead (MCP.tools connect cost etc.).
1004+
// altimate_change start (AI-7519) — trace resolveTools per step.
1005+
// Included in the parent `bootstrap` span on step===1; on later steps
1006+
// this measures the per-turn tool-listing overhead (MCP.tools connect
1007+
// cost etc.). Distinct span name per phase so telemetry doesn't
1008+
// double-count non-bootstrap turns under "bootstrap.*", and the TUI
1009+
// falls back to the safe "Thinking..." label on later turns.
10071010
const tools = await traceSpan(
1008-
"bootstrap.resolve-tools",
1011+
step === 1 ? "bootstrap.resolve-tools" : "turn.resolve-tools",
10091012
() =>
10101013
resolveTools({
10111014
agent,

packages/opencode/src/session/status.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,11 @@ export async function list() {
189189
// on entry (active=true) and exit (active=false); the TUI subscribes to render an honest label like
190190
// "Discovering warehouse tools..." during the pre-first-visible-response window. Publishes on both
191191
// the EventV2 bus (for V2 subscribers) and the LegacyEvent.Phase bus (for the SSE mirror the TUI
192-
// consumes). Best-effort: any failure here must not affect the traced operation itself.
192+
// consumes). Best-effort: any failure here must not affect the traced operation itself. The two
193+
// publishes are intentionally sequential (V2 first, legacy second) — an earlier attempt to
194+
// parallelise them via `Promise.allSettled` produced intermittent e2e failures where the label
195+
// wouldn't render, likely because the first ManagedRuntime warm-up races with the immediate
196+
// legacy Bus.publish. Sequential is reliable + the cost is negligible on the hot path.
193197
export async function publishPhase(sessionID: SessionID, phase: string, active: boolean) {
194198
try {
195199
await runStatus((s) => s.publishPhase(sessionID, phase, active))

packages/opencode/test/upstream/fork-feature-guards.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,11 @@ describe("fork feature presence guards (merge drop detection)", () => {
228228
expect(prompt).toMatch(/SessionStatus\.publishPhase\(sessionID, name, false\)/)
229229
expect(prompt).toMatch(/"bootstrap\.session-get",[\s\S]{0,120}sessionID/)
230230
expect(prompt).toMatch(/"bootstrap\.config-get",[\s\S]{0,120}sessionID/)
231-
expect(prompt).toMatch(/"bootstrap\.resolve-tools",[\s\S]{0,300}sessionID/)
231+
// resolve-tools span name is step-aware — "bootstrap.resolve-tools" on
232+
// step===1, "turn.resolve-tools" on subsequent steps so telemetry doesn't
233+
// over-count bootstrap operations.
234+
expect(prompt).toMatch(/"bootstrap\.resolve-tools"\s*:\s*"turn\.resolve-tools"/)
235+
expect(prompt).toMatch(/step\s*===\s*1[\s\S]{0,200}resolve-tools[\s\S]{0,400}sessionID/)
232236

233237
const sync = await read("src/context/sync.tsx", MONO + "/tui")
234238
expect(sync).toMatch(/session_phase:\s*\{/)

0 commit comments

Comments
 (0)