Skip to content

[6/6] Add Python goal cleanup integration coverage - #27113

Closed
aibrahim-oai wants to merge 1 commit into
dev/python-goal-controls-e2efrom
dev/python-goal-e2e
Closed

[6/6] Add Python goal cleanup integration coverage#27113
aibrahim-oai wants to merge 1 commit into
dev/python-goal-controls-e2efrom
dev/python-goal-e2e

Conversation

@aibrahim-oai

@aibrahim-oai aibrahim-oai commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Why

Goal operations outlive individual physical turns, so cancellation, terminal failures, stream closure, and transport loss must release routing and leave the thread usable.

What

Add real app-server integration coverage for:

  • terminal runtime failures and logical status preservation
  • sync and async cancellation during work
  • immediate async startup cancellation with background cleanup
  • stream closure and real transport termination
  • invalid objectives, unmaterialized threads, disabled goals, and active-thread rejection
  • router cleanup and ordinary turns after goal termination

This is a test-only PR and the final layer of the stack. It does not change production code, generated types, or generation scripts.

Test plan

  • online CI, including the Python SDK suite
  • all cases use a real app-server process and deterministic Responses test server; no SDK mocking

@aibrahim-oai

aibrahim-oai commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@aibrahim-oai aibrahim-oai changed the title Add Python goal end-to-end coverage Add Python goal operation end-to-end coverage Jun 9, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59fb22960c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

def interrupt_when_collecting() -> None:
harness.responses.wait_for_requests(1)
deadline = time.monotonic() + 5
while not goal_state._notifications.empty():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Fix inverted wait before interrupting

In the delayed SSE path, this loop exits immediately when _notifications is still empty, so the helper can call _thread.interrupt_main() before turn.run() has actually started consuming the goal stream. That makes the cancellation test depend on thread scheduling and can either miss the intended in-stream cancellation path or raise KeyboardInterrupt outside the pytest.raises block.

Useful? React with 👍 / 👎.

@aibrahim-oai aibrahim-oai changed the title Add Python goal operation end-to-end coverage [4/4] Add Python goal operation end-to-end coverage Jun 9, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74dd0e154b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


# Keep the event loop occupied until model work starts so the
# worker result cannot reach start_goal before cancellation.
harness.responses.wait_for_requests(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Badge Yield before waiting for async goal startup

Because this runs inside asyncio.run, the synchronous harness.responses.wait_for_requests(1) blocks the event loop immediately after asyncio.create_task(...); the thread.start_goal(...) coroutine cannot actually start and issue the request until the loop gets control again, so this test times out before it reaches the cancellation path. Await a non-blocking wait such as await asyncio.to_thread(harness.responses.wait_for_requests, 1) or otherwise yield before polling.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c35989186

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +120 to +121
harness.responses.wait_for_requests(2)
steer = turn.steer("Prioritize the edge-case coverage.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Block the continuation before steering

This only waits until the continuation request is recorded, but the queued continuation response is finite and keeps completing on the background mock server. If CI pauses this test for longer than the few delayed SSE chunks, the goal can roll into the next queued turn before turn.steer(...) runs, so the steer request targets the wrong physical turn or the requests[2] assertion observes the completion-tool turn instead. Hold the continuation open until after the control call, or synchronize on an in-stream state that cannot finish first.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2cbe607db1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

harness.responses.wait_for_requests(1)
with pytest.raises(InvalidRequestError) as error:
thread.start_goal("Do not replace active work")
ordinary.interrupt()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Hold the ordinary turn open before interrupting

If CI stalls after wait_for_requests(1), the mock SSE stream can finish all delayed chunks before this call runs. The app-server rejects interrupts for completed turns, so this test becomes timing-dependent instead of proving that an active ordinary turn survives the rejected goal start. Gate the response with a release event or otherwise synchronize on an in-flight state that cannot complete before ordinary.interrupt().

Useful? React with 👍 / 👎.

harness.responses.wait_for_requests(1)
process = codex._client._proc
assert process is not None
process.terminate()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Keep the stream in flight before killing the process

If CI pauses after wait_for_requests(1), this finite SSE response can finish before process.terminate() runs, so turn.run() may consume normal/completion or model-failure notifications instead of exercising the transport-death path and raising TransportClosedError. Hold the mock response open with a release event until after the process is terminated so the test deterministically kills an in-flight stream.

Useful? React with 👍 / 👎.

@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-public-api branch from 390ae2c to d26e9c7 Compare June 9, 2026 20:38
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-e2e branch from aabcee4 to 0ea8f80 Compare June 9, 2026 20:39

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ea8f80113

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +442 to +444
startup.cancel()
with pytest.raises(asyncio.CancelledError):
await startup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Release the SSE gate before awaiting startup cancellation

When the startup task is cancelled after the mock SSE response has reached this gate, AsyncCodex.start_goal_operation runs its cancellation cleanup and waits for the goal turn to reach a terminal notification before it re-raises CancelledError. Because the mock response is not released until this await startup finishes, the terminal event cannot be produced, so this test stalls until the cleanup timeout and then fails instead of exercising the cancellation path. Release the gate before awaiting the cancelled task, or release it from a finally inside the cleanup wait.

Useful? React with 👍 / 👎.

@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-public-api branch from d26e9c7 to e0a7af8 Compare June 9, 2026 22:34
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-e2e branch from f83e3a6 to 9da72f1 Compare June 9, 2026 22:34
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-public-api branch from 8d63303 to 3fbee02 Compare June 9, 2026 23:23
@aibrahim-oai
aibrahim-oai changed the base branch from dev/python-goal-public-api to dev/python-goal-controls-e2e June 9, 2026 23:23
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-e2e branch from 9da72f1 to 479a6b6 Compare June 9, 2026 23:23
@aibrahim-oai aibrahim-oai changed the title [4/4] Add Python goal operation end-to-end coverage [6/6] Add Python goal cleanup integration coverage Jun 9, 2026
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-e2e branch from 479a6b6 to edf5ddb Compare June 9, 2026 23:27
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-controls-e2e branch from e503a00 to 18ae017 Compare June 9, 2026 23:30
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-e2e branch from edf5ddb to 1b84c2a Compare June 9, 2026 23:30
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-controls-e2e branch from 18ae017 to 879c090 Compare June 9, 2026 23:32
@aibrahim-oai
aibrahim-oai force-pushed the dev/python-goal-e2e branch from 1b84c2a to 3d72c48 Compare June 9, 2026 23:32
@github-actions

Copy link
Copy Markdown
Contributor

Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR.

@github-actions github-actions Bot closed this Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant