Repository navigation
feat: retain provider response observations in native history - #5512
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 71ae120 after coordinator and two independent static reviews, including the saved-state security boundary.
Terminal observations preserve supplied status/reason/counters before existing completion/error handling, with native occurrence IDs distinct from provider IDs. Unknown counters remain unknown; normalized legacy accounting is labeled separately. Restore, fork, rollback and compaction retain reporting identities without making observations model input or execution authority. Failed required observation append propagates Fatal through sampling and local/remote compaction without issuing a replacement provider request; cancellation and ephemeral behavior remain explicit. Reviewed the regression coverage and patch manifest checksum. No actionable findings.
Hosted native CI is still in progress; change selection, lint and docs have passed. No candidate code or local tests executed. Retained observations do not establish complete billing history; older readers may reject new snapshot records as documented.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
New hosted evidence exposes a deterministic failure in the response-history test introduced here, inherited by #5508. Requesting the focused test-contract correction below; this does not establish a new runtime status regression. Coordinator and independent static investigation; no candidate code or local tests executed.
|
Hosted CI follow-up at |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The regression now accepts both existing SDK settlement outcomes (failed or cancelled) while still requiring NativeError, provider-completed history, received usage, and exactly one request. This resolves the previous assertion finding; the separate hosted timeout remains a CI follow-up. Coordinator and independent static review complete. Current-head hosted checks: {'SKIPPED': 4, 'SUCCESS': 3, 'IN_PROGRESS': 2}. Native/SDK checks remain pending. No candidate code or local tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
New hosted evidence reveals missing provider response history in the rejection path. Requesting changes on this distinct finding; the prior status-assertion correction remains valid. Coordinator and independent static review; no candidate code or local tests executed.
230f9ee to
0013cdf
Compare
7964d53 to
efa3e9a
Compare
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The observation acknowledgment resolves my earlier rejection-path history finding: the stream mapper cannot process the terminal interceptor rejection until Core has persisted the preceding provider observation. The acknowledgment is one-shot across pending polls, and consumer drop releases a waiting producer. Persistence failure cancels and joins this sample's tools without cancelling the owning turn or replacing the fatal retention error. Source review also verified retained receipts, response facts and late execution rows coexist after base integration. Coordinator and independent static re-review found no new actionable issue. Hosted native/SDK CI remains pending; no candidate code or local tests executed.
|
The macOS follow-up is now verified at The test cleanup guard also releases the held callback before the session closes when an assertion fails, preserving the original error instead of masking it with a cleanup timeout. All reported current-head CI checks are terminal and green. |
efa3e9a to
a776c72
Compare
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved after coordinator and independent static re-review of the base integration. Direct tree comparison shows 97 of 102 changed files equal the current base. The remaining overlap preserves the existing response-history projection and test registration; the native observation patch changes only rebased context/whitespace, retaining the previously reviewed receipt-ordering fix.
Hosted CI remains pending. Static review only; no candidate code or local tests ran.
Observe passive request metadata on repaired history Co-authored-by: markstuart-oai <[email protected]>
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved after static re-review. Saved-login and previously reviewed passive-request metadata integration preserves response-observation routing and the existing feature patch. Hosted CI/native builds remain pending. No candidate code or tests executed.
|
Hosted CI at merged 58d072c: macOS sandbox job failed test_stop_before_selected_result_returns_settles_without_stale_approval[True] at tests/test_web_starter.py:569 while waiting for initial pending approval, before the stop assertions. The test is byte-identical to base; logs do not establish a candidate-specific cause. 3916 passed, one failed. Other CI remains pending. No local tests or CI reruns performed. Update 2026-11-05 09:03 UTC: Read hosted results only; no local candidate code/tests or CI reruns. |
Summary
Requirement: REQ421 — retain received response identity, terminal status and supplied usage through history and persistence; broader route coverage and accounting acceptance remain separate.
Native history stores the final SDK result and total usage, but can lose details from earlier provider responses. A later success can hide an incomplete response that triggered a retry.
Store received terminal response observations with a stable occurrence ID, provider response ID, status, reason, and supplied usage counters. Link usage records to those observations and preserve them through save, restore, and fork. Fail the operation when required recording fails, without issuing another provider request; preserve cancellation. Existing snapshots still load through normalization.
Missing counters remain unknown, so retained observations alone cannot establish complete billing history. Older SDKs may reject newly saved snapshots.
The merged passive metadata slice supports REQ742: direct-root dispatch facts link to the retained response occurrence without exposing request bodies, headers or credentials.
Test plan
58d072c8includes saved-login v1 and the approved passive metadata changes merged from feat: expose passive request metadata and response links #5519. All 47 Core patches replay unchanged. Normal source preparation, API generation, Ruff and 40 pure history/metadata tests passed.Issue number
None linked.
Checks
AGENTS.md, including.agents/skills/code-change-verification/scripts/run.shwhen requiredAGENTS.md