Repository navigation
Reconcile uncertain local tool calls before resuming - #5502
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
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at b15f244. Reviewed the reconciliation layer against its immediate base: application receipts must cover every unresolved invocation, match native thread/turn/call identities, and agree with retained results. Durable import precedes replay fencing; descendants and executor-policy mismatches remain guarded. No actionable findings.
Static review only; no candidate code or tests run. Hosted lint/docs checks passed; Linux/macOS native CI remains running and Windows checks are skipped. Leave unmerged as requested.
759b4ec to
7ceb5da
Compare
b15f244 to
d2f6580
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at d2f6580.
Reviewed the recovery API update with an independent reviewer. UncertainCall.reconcile preserves the original identity and delegates validation to ReconciledCall without side effects. Regression changes cover invalid confirmations, journal equivalence, repeated reopen and replay fencing. No actionable findings in this layer; underlying executor blockers remain tracked on5501.
Static review only; no candidate code or tests run. Fresh hosted CI remains pending. Leave unmerged as requested.
7ceb5da to
99f4b1c
Compare
d2f6580 to
94defa9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94defa9c8b
ℹ️ 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.
Confirmed the existing recovery finding: #5502 (comment). Custom-tool input such as 42, null, or {} is projected as function arguments, then JSON-decoded in _uncertain_call, changing the original input before an application verifies the external effect. Preserve the call kind and decode only function-call arguments; retain custom-tool input verbatim. This supersedes my earlier approval. No duplicate inline comment added.
Static review only; no candidate code or tests executed. Hosted CI: pending: native-linux / Native package (ubuntu-24.04), native-linux / Native package (ubuntu-24.04), native-macos / Native package (macos-latest), native-macos / Native package (macos-latest); no failed checks observed.
99f4b1c to
82035ad
Compare
94defa9 to
109e6e0
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The custom-tool recovery input finding is fixed. The history projection preserves raw custom input separately through native item replacement, including empty and JSON-shaped strings, while function arguments retain normal JSON decoding. No remaining actionable finding in this layer.
Static review only; no candidate code or tests executed. Hosted CI: pending: native-linux / Native package (ubuntu-24.04), native-linux / Native package (ubuntu-24.04), native-macos / Native package (macos-latest), native-macos / Native package (macos-latest); no failed checks observed.
109e6e0 to
71775f5
Compare
82035ad to
162be16
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact tree changes and inherited executor updates. No additional issue in this layer. One existing P2 remains: #5501 (comment). For an implicit WSS Environment without network policy, a pending manual turn explicitly limited to tools=[manual_tool] still captures an executor-policy digest solely because active is non-null. Its saved operation_tools already excludes executor capabilities, yet restore without the unused Environment fails with Checkpoint and Environment must agree. Gate the active-turn digest requirement on the operation actually selecting executor tools; retain strict-write and network-policy checks. No duplicate inline finding.
The sandbox-denial output report is not a blocker: result-policy capture marks the call before execution, the global completed-item filter withholds output before observers/history, and the error path returns the withheld failure.
Static review only; no candidate code or tests executed. Hosted checks pending: native-linux / Native package (ubuntu-24.04), native-linux / Native package (ubuntu-24.04), native-macos / Native package (macos-latest), native-macos / Native package (macos-latest). Failed checks: none observed.
162be16 to
ff47ea4
Compare
954eb25 to
a5ae33d
Compare
22e656e to
deb5647
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The shared physical write-root and storage protections are sound: filesystem identities cover alternate paths, checklist aliases and hard links are checked, and private runtime symlinks fail closed. Disjoint storage and write grants remain supported. The executor startup fixture also removes the parent-selected port race. Reconciliation behavior is unchanged in the actual old/new blob diff.
Coordinator and independent static reviews of actual old/new file contents found no actionable issue. Hosted CI on this new head remains pending; prior-head results do not qualify it. No candidate code or local tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-approved f7dae0bc4f1a64fd858473b2da319cf8812a9836. Only the shared deep-path hard-link scanner patch and its manifest changed; both exactly match the current base and the previously reviewed fix. Recovery implementation is unchanged. No new actionable findings from coordinator and independent static review.
Fresh hosted native CI remains pending. No candidate code, builds or local tests executed. Existing merge holds remain in effect.
Review stack: #5504 (CI) > #5499 (turn controls) > #5500 (receipts) > #5501 (executor) > #5502 (recovery) > #5503 (example) > #5495 (docs). Combined reference: #5494.
Requirements ·
REQ733.Summary
Recovery preserves the exact text received by a custom tool. It decodes JSON for ordinary function calls.
Once the application records an
appliedornot_appliedoutcome, it can reopen the session without submitting that receipt again. This also covers histories that retain a native tool start but lack its native completion.The SDK reconciles uncertain calls from the root session. If an uncertain effect belongs to a child, recovery stops before exposing a receipt that the root cannot reconcile.
A process can exit after a tool takes effect but before its result is recorded. Reopening that history must stop before another model request or automatic replay. Add
LocalRecoveryRequiredwith the uncertain calls and retained history, plusReconciledCallinput for outcomes the application verified independently.UncertainCall.reconcile(outcome=..., result=...)constructs that confirmation from the reported call's IDs, without requiring the caller to copy them.Bind each supplied result to the original native thread, turn and call under the local storage writer. Flush and read back the result before admitting new work. Retained reconciliation tags prevent replay of those recorded invocation IDs. The application owns effect reconciliation and duplicate prevention across new call IDs; this does not provide general exactly-once execution or undo effects. Recovery requires a supported idle local session; cold descendant recovery remains unsupported.
Layer 4 of 5; base
feat/worker-executor. It uses the executor and retained-record contracts from the earlier layers. Model auth remains API-key-only; managed login/billing are excluded underREQ731.Acceptance dependencies enabled here:
REQ781needs crash reconciliation, andREQ780needs uncertain-effect reconciliation before another publication decision. This layer supplies recovery controls; it does not establish application acceptance.Test plan
The old projection asked for another receipt on a second ordinary reopen for both outcomes. The fix passed 27 recovery and history tests, including both receipt-free reopens, retrying an identical receipt, and fencing reuse of the original call ID.
All 27 native storage and descendant recovery cases passed. Five focused cases covered JSON-looking custom text, empty input and mixed raw/native history records. Ruff and the focused type checks passed.
Issue number
None linked. Owning requirements database IDs are listed above.
Checks
AGENTS.md, including.agents/skills/code-change-verification/scripts/run.shwhen requiredAGENTS.md