Skip to content

fix(test): restore abort-and-lifecycle stdin-close test to pre-#3723 version #3777

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
wenshao merged 4 commits into from
May 2, 2026
Merged

fix(test): restore abort-and-lifecycle stdin-close test to pre-#3723 version #3777

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88b3e1b
fix(test): restore abort-and-lifecycle stdin-close test to pre-#3723 …
wenshao Apr 30, 2026
b4e4dd6
test(integration): add timeout and unify loop into race chain
wenshao Apr 30, 2026
432bbf7
test(integration): close pseudo-pass paths in stdin-close lifecycle test
wenshao May 1, 2026
bf82093
test(integration): defer phase timers in stdin-close lifecycle test
wenshao May 1, 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
56 changes: 39 additions & 17 deletions integration-tests/sdk-typescript/abort-and-lifecycle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,23 +314,38 @@ describe('AbortController and Process Lifecycle (E2E)', () => {
});

it('should handle control responses when stdin closes before replies', async () => {
const testFilePath = await helper.getPath('test.txt');
await helper.createFile('test.txt', 'original content');

let canUseToolCalled = false;
let canUseToolCalledResolve: () => void = () => {};

@wenshao wenshao Apr 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Suggestion] These new timeout handles are not cleared after the promises resolve. On successful runs the real timers can remain live until they fire, adding latency or open-handle noise to the integration test worker. Please store each timeout handle and clear it in the corresponding resolve path, or use a helper that clears on settle.

— gpt-5.5 via Qwen Code /review

const canUseToolCalledPromise = new Promise<void>((resolve) => {
const canUseToolCalledPromise = new Promise<void>((resolve, reject) => {
canUseToolCalledResolve = resolve;
setTimeout(() => {
reject(new Error('canUseTool callback not called'));

@wenshao wenshao Apr 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Critical] The targeted E2E test failed in verification because canUseTool was never called before this new timeout rejected on every retry. That means this change is not verified green and still depends on reaching a model/tool-call path that may not happen in the test environment. Please make the test deterministic enough to reliably reach the intended permission callback path, or adjust the lifecycle assertion so it does not rely on an environment-coupled model tool call.

— gpt-5.5 via Qwen Code /review

}, 15000);
});

@wenshao wenshao May 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Suggestion] canUseToolCalled and inputStreamDone have 15 s budgets that start counting at creation, but they are logically awaited only after firstResult (30 s budget). On a slow CI where the first LLM round-trip exceeds 15 s, these promises will have already rejected before the sequential chain reaches them, producing a misleading "canUseTool callback not called timeout" error when the real cause was first-result latency.

Consider matching the 15 s budgets to 30 s, or deferring promise creation until just before the relevant phase starts.

— glm-5.1 via Qwen Code /review

let inputStreamDoneResolve: () => void = () => {};
const inputStreamDonePromise = new Promise<void>((resolve, reject) => {
inputStreamDoneResolve = resolve;
setTimeout(() => {
reject(new Error('inputStreamDonePromise timeout'));
}, 15000);
});

let firstResultResolve: () => void = () => {};
const firstResultPromise = new Promise<void>((resolve) => {
const firstResultPromise = new Promise<void>((resolve, reject) => {
firstResultResolve = resolve;
setTimeout(() => {
reject(new Error('firstResult not received within 30s'));
}, 30000);
});

let secondResultResolve: () => void = () => {};
const secondResultPromise = new Promise<void>((resolve, reject) => {
secondResultResolve = resolve;
setTimeout(() => {
reject(new Error('secondResult not received within 30s'));
}, 30000);
});

async function* createPrompt(): AsyncIterable<SDKUserMessage> {
Expand All @@ -353,10 +368,12 @@ describe('AbortController and Process Lifecycle (E2E)', () => {
session_id: sessionId,
message: {
role: 'user',

@wenshao wenshao Apr 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Suggestion] The prompt now uses relative test.txt instead of the absolute helper path. If the model/tool resolves that name against a different working directory or target, the helper-managed file can remain unchanged and this assertion can pass without proving the intended file's pending write was rejected after stdin closed. Please restore const testFilePath = await helper.getPath('test.txt') and prompt with that absolute path.

— gpt-5.5 via Qwen Code /review

content: `Use the write_file tool to write "updated" to the file at ${testFilePath}. Then reply with "done".`,
content:
'Write "updated" to test.txt. Stop if any exception occurs.',
},
parent_tool_use_id: null,
};
await inputStreamDonePromise;
}

const q = query({
Expand All @@ -367,8 +384,10 @@ describe('AbortController and Process Lifecycle (E2E)', () => {
permissionMode: 'default',
coreTools: ['read_file', 'write_file'],
canUseTool: async (toolName, input) => {

@wenshao wenshao Apr 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Critical] This callback resolves the synchronization promises for any tool call, but coreTools includes both read_file and write_file. If the model calls read_file, or calls write_file with an unexpected path/content, the file can remain original content and the test still passes without exercising the intended delayed write_file permission-response path. Please assert toolName === 'write_file' and validate the expected target/content before resolving these promises.

— gpt-5.5 via Qwen Code /review

canUseToolCalled = true;
inputStreamDoneResolve();
await new Promise((resolve) => setTimeout(resolve, 1000));
canUseToolCalledResolve();

return {
behavior: 'allow',
updatedInput: input,
Expand All @@ -381,8 +400,10 @@ describe('AbortController and Process Lifecycle (E2E)', () => {
try {
const loop = async () => {
let resultCount = 0;
for await (const message of q) {
if (isSDKResultMessage(message)) {
for await (const _message of q) {
console.log(JSON.stringify(_message, null, 2));

@wenshao wenshao May 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Suggestion] This unconditional console.log(JSON.stringify(_message, null, 2)) logs every SDK message. On CI retries this can produce noisy output and make the relevant lifecycle failure harder to scan.

Please remove the unconditional log, or gate compact diagnostics behind an explicit debug flag.

— gpt-5.5 via Qwen Code /review

// Consume messages until completion.
if (isSDKResultMessage(_message)) {

@wenshao wenshao May 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Critical] The loop treats any second SDKResultMessage as success, while the final assertion only checks that the file stayed unchanged. If the second turn ends with subtype: 'error_during_execution' because the delayed permission/control response is dropped after stdin closes, the file remains unchanged and the test still passes.

Please capture the second result and assert the intended outcome explicitly, for example subtype === 'success' plus a result/tool observation that proves the delayed control response was handled, instead of using unchanged file content alone as the success condition.

— gpt-5.5 via Qwen Code /review

resultCount += 1;
if (resultCount === 1) {
firstResultResolve();
Expand All @@ -395,18 +416,19 @@ describe('AbortController and Process Lifecycle (E2E)', () => {
}

@wenshao wenshao May 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Critical] Promise.race([expectedSequence, loopPromise]) lets the test proceed successfully if the SDK iterator ends before canUseToolCalledPromise or secondResultPromise resolves. In that case the final file-content assertion can pass without exercising the intended stdin-close/control-response path, so the lifecycle regression this test is meant to catch can be masked.

Please do not treat normal loopPromise completion as a success condition here. Await the required milestones directly, or make loop() reject if it exits before the expected second result/control callback is observed.

— gpt-5.5 via Qwen Code /review

};

loop();
const loopPromise = loop();

await firstResultPromise;
await canUseToolCalledPromise;
await secondResultPromise;

// Signal stdin is done so CLI stops waiting
q.endInput();
await Promise.race([
(async () => {
await firstResultPromise;
await canUseToolCalledPromise;
await secondResultPromise;
})(),
loopPromise,
]);

const content = await helper.readFile('test.txt');
expect(canUseToolCalled).toBe(true);
expect(content).toBe('updated');
expect(content).toBe('original content');
} finally {
await q.close();
}
Expand Down
Loading