Skip to content

fix(test): deflake tool.shell metadata streaming test (LAC-2722) #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lacymorrow merged 2 commits into from
Jul 13, 2026
Merged

fix(test): deflake tool.shell metadata streaming test (LAC-2722) #40

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a69e290
fix(test): deflake tool.shell metadata streaming test (LAC-2722)
lacymorrow Jul 13, 2026
47867e0
fix(test): make streaming fixture cross-shell and gate write idempote…
lacymorrow Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 43 additions & 23 deletions packages/opencode/test/tool/shell.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { PermissionV1 } from "@opencode-ai/core/v1/permission"
import { describe, expect } from "bun:test"
import { Cause, Effect, Exit, Layer } from "effect"
import type * as Scope from "effect/Scope"
import fs from "fs"
import os from "os"
import path from "path"
import { Config } from "@/config/config"
Expand Down Expand Up @@ -1100,29 +1101,48 @@ describe("tool.shell abort", () => {
),
)

it.live("streams metadata updates progressively", () =>
runIn(
projectRoot,
Effect.gen(function* () {
const updates: string[] = []
const result = yield* run(
{
command: `echo first && sleep 0.1 && echo second`,
},
{
...ctx,
metadata: (input) =>
Effect.sync(() => {
const output = (input.metadata as { output?: string })?.output
if (output) updates.push(output)
}),
},
)
expect(result.output).toContain("first")
expect(result.output).toContain("second")
expect(updates.length).toBeGreaterThan(1)
}),
),
it.live(
"streams metadata updates progressively",
() =>
runIn(
projectRoot,
Effect.gen(function* () {
// The second chunk is gated on the test observing the first update, so the
// two writes can never coalesce into a single flush under runner load.
const gate = path
.join(os.tmpdir(), `shell-stream-gate-${process.pid}-${Math.random().toString(36).slice(2)}`)
.replaceAll("\\", "/")
// String.fromCharCode keeps the eval'd code free of quote characters so it
// survives evalarg's quoting under every shell (bash, pwsh, cmd).
const js = (text: string) => `String.fromCharCode(${[...text].map((c) => c.charCodeAt(0)).join(",")})`
const code = `const f=require(${js("fs")});f.writeSync(1,${js("first\n")});while(!f.existsSync(Bun.argv[1]))Bun.sleepSync(50);f.writeSync(1,${js("second\n")})`
const text = `${bin} -e ${evalarg(code)} ${quote(gate)}`
const updates: string[] = []
let gateWritten = false
const result = yield* run(
{
command: PS.has(sh()) ? `& ${text}` : text,
timeout: 10_000,
},
{
...ctx,
metadata: (input) =>
Effect.sync(() => {
const output = (input.metadata as { output?: string })?.output
if (output) updates.push(output)
if (output?.includes("first") && !gateWritten) {
gateWritten = true
fs.writeFileSync(gate, "")
}
}),
},
).pipe(Effect.ensuring(Effect.sync(() => fs.rmSync(gate, { force: true }))))
Comment on lines +1120 to +1139

@gemini-code-assist gemini-code-assist Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since output is cumulative, output?.includes("first") will evaluate to true for every metadata update after the first one. This causes fs.writeFileSync(gate, "") to be called repeatedly on every subsequent chunk, performing redundant synchronous disk I/O.

We can optimize this by introducing a local boolean flag (gateWritten) to ensure the gate file is only written once.

          let gateWritten = false
          const updates: string[] = []
          const result = yield* run(
            {
              command: "echo first; until [ -f \"" + gate + "\" ]; do sleep 0.05; done; echo second",
              timeout: 10_000,
            },
            {
              ...ctx,
              metadata: (input) =>
                Effect.sync(() => {
                  const output = (input.metadata as { output?: string })?.output
                  if (output) updates.push(output)
                  if (output?.includes("first") && !gateWritten) {
                    gateWritten = true
                    fs.writeFileSync(gate, "")
                  }
                }),
            },
          ).pipe(Effect.ensuring(Effect.sync(() => fs.rmSync(gate, { force: true }))))

@lacymorrow lacymorrow Jul 13, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 47867e0 — added a gateWritten flag so the gate file is written exactly once.

expect(result.output).toContain("first")
expect(result.output).toContain("second")
expect(updates.length).toBeGreaterThan(1)
}),
),
15_000,
)
})

Expand Down
Loading