fix(sdk): preserve caller cancellation in cloud publication - #677
Conversation
|
Focused regression tests and repository formatting checks pass on the current head ( |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Thanks for keeping this small and covering both cancellation points! I ran the Cloud publisher and CLI suites: all 66 tests passed, along with typechecking.
The added cancellation test needs a small Prettier cleanup before CI can pass: run pnpm exec prettier --write tests-ts/cloud-publish.test.ts from sdk/typescript. The required adjustment is format-only.
Approving the cancellation-identity change. One follow-up worth retaining: an interrupted upload may already have been accepted, so the CLI should keep the check-before-retrying guidance and a useful batch error instead of only SIGINT/SIGTERM. The existing CLI tests mock the publisher, so a test connecting the real publisher to the CLI would help cover that interaction.
|
Thanks again for the focused cancellation fix! I carried the change into #680 with the original author attribution, the test-formatting cleanup, and coverage connecting the real publisher to the CLI. That follow-up preserves the caller cancellation reason while keeping the check-before-retrying guidance and useful batch failure output. |
|
Thank you for carrying this into #680 and preserving the original author attribution. I confirmed that #680 includes the cancellation propagation plus the CLI-level recovery guidance and integration coverage you described. I also pushed the requested Prettier-only cleanup to this branch. On the current head, the focused Cloud publication file passes (35 tests), the full CI suite passes (1,917 passed, 30 platform-specific skips, 0 failures), and the packed/installed-package checks pass. Since #680 supersedes this branch, I will avoid duplicating its follow-up changes and leave closure to the maintainers. |
mldangelo-oai
left a comment
There was a problem hiding this comment.
Thanks for the formatting follow-up and the additional validation! I verified that this head is identical to the locally validated formatting fix, with no production changes beyond the original cancellation patch. Approving this exact head. The CLI recovery-guidance follow-up is tracked in #680, including cancellation before and during an upload.
Summary
Caller cancellation during cloud publication is currently caught and rethrown as a generic
CodexSecurityError. This loses the caller's cancellation identity and can make cancellation look like a publication failure.Reproduction:
AbortSignal.Expected: the original cancellation error is propagated unchanged.
Actual: the generic cloud-publication error wrapper is thrown instead.
Changes
Testing
bun test --timeout 30000 ./tests-ts/cloud-publish.test.ts(35 passed)pnpm run typespnpm run formatpnpm run test:ci(1,917 passed, 30 platform-specific skips, 0 failed)pnpm pack --pack-destination ../../distpnpm run check:package /tmp/codex-security-verify/dist/openai-codex-security-0.1.20.tgzpnpm run test:packagegit diff --checkThe package archive inspection and installed-package smoke test validated the public import, NodeNext types, CLI, credential locking, bundled plugin files, bundled Codex version, and a nested worker without a global
codexexecutable.Risk and rollout
This preserves the existing cancellation contract without changing the public API or successful publication behavior. Non-cancellation failures continue through the existing
CodexSecurityErrorhandling.Public disclosure review