fix(test): deflake tool.shell metadata streaming test (LAC-2722)#40
Conversation
The 'streams metadata updates progressively' test raced the scheduler: under runner load both echo chunks arrived within one flush window and coalesced into a single metadata update, failing expect(updates.length).toBeGreaterThan(1). Gate the second chunk on the test observing the first update via a tmpdir sentinel file, so two distinct updates are guaranteed by construction rather than by timing. Co-Authored-By: Paperclip <noreply@paperclip.ing>
cd83bb1 to
a69e290
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the progressive metadata streaming test in shell.test.ts to use a file-based gating mechanism, ensuring the test is robust under runner load. Feedback suggests optimizing the gate-writing logic to avoid redundant synchronous disk I/O since the output is cumulative, and addressing potential cross-platform failures on Windows due to the POSIX-specific shell syntax used in the test command.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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")) fs.writeFileSync(gate, "") | ||
| }), | ||
| }, | ||
| ).pipe(Effect.ensuring(Effect.sync(() => fs.rmSync(gate, { force: true })))) |
There was a problem hiding this comment.
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 }))))There was a problem hiding this comment.
Done in 47867e0 — added a gateWritten flag so the gate file is written exactly once.
| const updates: string[] = [] | ||
| const result = yield* run( | ||
| { | ||
| command: `echo first; until [ -f "${gate}" ]; do sleep 0.05; done; echo second`, |
There was a problem hiding this comment.
The command uses POSIX-specific shell syntax (until [ -f ... ]; do sleep 0.05; done). This will fail with syntax errors on Windows if the default shell is PowerShell or CMD (which is common on Windows development environments where the SHELL environment variable is not set).
Consider wrapping this test with withShell({ label: 'bash', shell: bash }, ...) if bash is defined, or using a cross-platform node/bun one-liner to ensure compatibility across all platforms.
There was a problem hiding this comment.
Good catch — on win32 with no SHELL set, Shell.acceptable() falls back to pwsh/powershell, where until [ -f ... ] doesn't parse. Fixed in 47867e0 by replacing the shell loop with a bun -e one-liner built the same way as the existing fill helper (charcode-encoded, & -prefixed for PowerShell, evalarg quoting), so the fixture runs under bash, pwsh, and cmd.
…nt (LAC-2722) Address Gemini review on PR #40: - Replace POSIX-only 'until [ -f ... ]' loop with a bun -e one-liner (charcode-encoded like the existing fill helper) so the fixture parses under bash, pwsh, and cmd on Windows runners - Write the gate file once via a gateWritten flag instead of on every cumulative metadata update
…-2026-07-13 * origin/dev: fix(test): deflake tool.shell metadata streaming test (LAC-2722) (#40) fix(llm): break circular type inference in ToolResultValue schema fix(build): update worker entrypoint path after upstream sync fix: restore release script lost in upstream sync merge
Summary
Fixes the flaky
tool.shell abort > streams metadata updates progressivelytest (LAC-2722) seen on GitHub-hosted linux runners, e.g. run 29081382435.Problem
The test asserted
updates.length > 1for a command emitting two output chunks separated bysleep 0.1. Under full-suite load, both chunks arrived within one flush window and coalesced into a single metadata update — the assertion was racing the scheduler, not testing the streaming contract.Fix
Make the producer deterministic instead of raising thresholds: the fixture command now emits the second chunk only after the test observes the first metadata update, signaled via a tmpdir sentinel file written from the metadata callback. Two distinct updates are guaranteed by construction, independent of runner load.
Also adds an explicit 10s command timeout and 15s test timeout so a genuine streaming regression fails cleanly instead of hanging on the gate.
Testing
bun test test/tool/shell.test.ts -t 'streams metadata updates progressively'— 10 consecutive sequential passestest/tool/shell.test.ts— 23 pass / 0 fail (3 consecutive runs)bun run typecheckinpackages/opencode— cleanAcceptance criteria