Repository navigation
Add SessionStore for durable native sessions and tool results - #5514
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (4)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 4ba33c3a11
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ba33c3a11
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46710bdf93
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Requesting changes at 46710bd. Coordinator and independent static reviews confirmed these existing findings remain actionable; please address their existing threads (no duplicate inline comments):
- Retain the initialization writer token when release fails so cleanup can retry.
- Use the database identity for exclusive locks; symlink aliases currently admit concurrent writers.
- Retain child receipt commits across cancellation, as the root path already does.
- Do not register managed calls before enqueue succeeds; rejected callbacks currently leave false unknown effects.
- Make metadata persistence failures sticky at the canonical durability barrier.
- Preserve structured tool results and success/failure metadata during reconciliation.
- Create the SQLite database with private permissions; the default umask can expose conversation/tool data.
Hosted CI: still running: native-linux / Native package (ubuntu-24.04), tests-windows (3.10), tests-windows (3.14), native-macos-sandbox; no failures currently reported. No candidate code or local tests ran. Review only; no code changes or merges performed.
markstuart-oai
changed the title
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved after coordinator and independent static re-review at 818c8dc. All seven prior findings are addressed: failed initialization cleanup retains ownership for retry; SQLite paths are canonicalized and hard links rejected; child receipt commits and replies survive waiter cancellation; managed calls register only after enqueue; metadata failures remain sticky across later flushes; reconciliation preserves validated structured ToolOutput and success; database and sidecar permissions are restricted.
Reviewed the SessionStore/CustomStorage/SQLiteSessionStore API rename, native patch integration and focused regressions. The supervised-worker/history/recovery changes match the current base; custom-storage integration preserves unresolved-call rejection and local reconciliation boundaries. The unreleased hosted documentation edits were also removed. No new actionable findings.
Current-head hosted native builds remain pending; this is not a CI-green claim and does not establish resolution of the prior-head macOS timeout. No candidate code or local tests ran. This supersedes the prior request-changes decision.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 818c8dccd5
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the ownership fix and base integration. Existing database, sidecar, and lock ownership is checked before permission repair or canonical writes; database inspection no longer closes an extra descriptor that can release SQLite process locks. WAL and rollback-journal coverage supports the change. No new findings. Hosted CI is pending; the prior supervised-worker deadline failures still require a passing current-head run. Static review with independent review; no local tests executed.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: ae2e141341
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed d2916d38657a88ab1d8ae8be0b1ee8bbcb8aca7b: approved. Existing databases, SQLite sidecars and writer locks with group/other permissions are rejected before writes instead of having permissions repaired; private creation and owner/inode checks remain. This addresses #5514 (comment) . Reviewed retained-reader regressions and SQLite locking constraints statically. Current native CI is running; this does not claim earlier hosted failures are resolved. No local tests run.
|
Hosted CI follow-up at d2916d3: all five Linux Python-version jobs fail test_handoff_deadline_includes_submission[effect/recover]. After the initial brief is interrupted, reopening returns recovery_required during startup, so the fixtures do not reach their expected deadline/completion. Please reconcile the recovery handling/test setup. Representative log: https://github.com/openai/openai-agents-python/actions/runs/41978450704/job/127100663417 Windows separately fails test_child_cancel_withholds_private_function_cleanup_exception_chains[True-True] because child.turn is None before cancellation; the privacy assertions are not reached, so this is not evidence of a privacy leak. Log: https://github.com/openai/openai-agents-python/actions/runs/41978450704/job/127099846996 Other hosted checks remain pending. This is a hosted-CI report, not a new static code finding; no local tests were run. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fde490daf
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
I confirm the existing P1 manual-result durability finding. PythonExecutor registers manual calls as managed; LocalChecklistStore suppresses their Core FunctionCallOutput, but respond_manual sends the reply without commit_sdk_receipt. A successful live manual result is therefore absent from LocalStorage history. Route manual replies through the durable receipt gate before suppressing Core output. The fork-lock and deadline-fixture changes look correct. Static and independent review; hosted CI pending; no candidate code or tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The durable manual receipt gate fixes the prior missing-history finding, including commit-before-delivery and write-failure handling. One new cancellation gap remains in restored manual continuations. Static and independent review; native CI pending; no candidate code or tests executed.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 7fc84af807
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The cancellation finding is addressed: an owned delivery task now spans native reply acceptance and restored-turn continuation, and caller cancellation joins that task before propagating. The acceptance/cancellation regression covers single delivery and successful resumption. Storage documentation clarifies trusted ancestor and symlink requirements. Static and independent review; hosted CI pending. No candidate code or tests executed.
|
Hosted CI at merged 3887d39: mypy-win32 reports three errors introduced by canonical_storage.py: line 59 uses os.register_at_fork, and line 328 uses fcntl.flock and LOCK_UN, which are unavailable in Windows type stubs. The adapter restricts runtime use to POSIX, but its guards also need to narrow these optional APIs for Windows static analysis. Please make those accesses type-safe without weakening the existing runtime restriction. No candidate code, local tests or CI reruns executed. |
|
Additional hosted CI failure at merged 3887d39: all five Linux versions and macOS fail both test_handoff_deadline_includes_submission cases with Local resume requires the recorded executor write and network policy (representative Python 3.12 job). The changed seed at tests/test_supervised_worker_example.py:472-474 opens LocalStorage without an environment, while the worker resumes with Environment.local(cwd=workspace, writable_roots=(workspace,), tools=(exec_command,)) at examples/supervised_worker/main.py:389-410. Seed the history with that same executor policy so these tests reach their intended deadline assertions. macOS also has the inherited retrieval-startup timeout; the earlier Windows type-check errors remain separately tracked. No candidate code, local tests or CI reruns executed. |
Summary
Applications can now supply a
SessionStorethroughCustomStoragefor native Sessions.SQLiteSessionStoreprovides a persistent POSIX reference adapter. Reopening retains session, turn and call identities while using current application tools, context and credentials.The runtime records tool intent before dispatch and actual callback results before dependent work. Writer fencing, revision checks and stable write identities protect canonical updates. Storage errors remain visible through close. Unknown tool outcomes block automatic resume.
CustomStorage.reconcile()records an externally verified string orToolOutputfor the original call without rerunning it. Shutdown joins accepted child completion records before releasing writers.LocalStoragemanual tool replies also commit their actual result before dependent model work. A receipt write failure stops the reply and remains visible through close. The Session resumes a restored turn after its final manual reply, even if the caller cancels while the reply is being delivered.The preview supports root Sessions with automatic Python tools and rejects unsupported combinations before execution.
LocalStoragealso rejects persistent code mode until individual nested effects have durable intent and result receipts. Distinct Session IDs can share the reference database. Symlinks share the same writer locks; hard-linked databases are refused. The adapter rejects unsafe existing database, SQLite sidecar and writer lock files before writes. Existing files must be regular, single-linked, owned by the effective user and free of group/other permissions. File checks preserve SQLite write locks. Writer release and parent-process death also free ownership when a forked worker remains alive. Use trusted, stable pathnames, ancestor directories and symlink targets. Create history privately; changing modes cannot revoke prior readers. The example documents the backend contract and recovery limits. Behavioral hosted docs remain in a separate release-timed change.Test plan
3887d39: required formatting, lint, mypy and both Pyright checks passed. The full suite passed 4,004 parallel tests and 41 serial tests, with 173 environment/platform skips.Issue number
No public issue is linked.
Checks
AGENTS.md, including.agents/skills/code-change-verification/scripts/run.shwhen requiredAGENTS.md