Repository navigation
docs: clarify local shell approval scope across handoffs - #5385
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. |
![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: 210de1e738
ℹ️ 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".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 210de1e7385a8d8b654ab01fa014de12f3213dc9 against 17d994bd3045dae71acda676a4ef4762ed8ecd94.
I confirmed the existing approval-lifetime finding as P2. A separate Runner.run(...) call can inherit sticky shell approval when it reuses the same RunContextWrapper. The warning should describe the wrapper/RunState lifetime and explain that a separate approval domain needs a fresh wrapper, or only the application context passed to the new run. Keep the caveat that resuming a RunState preserves its decisions: per-call settings do not clear an existing sticky grant.
The handoff behavior and function-tool distinction otherwise match the implementation. Extending the warning to ApplyPatchTool would be useful, but I do not consider its omission a separate blocker in this patch.
Validation: both changed documents and the relevant approval, context, serialization, and test source; 20 hosted checks passed and 5 skipped at this head. Source-only review; I did not run candidate code or a documentation build.
|
I've resolved the review comments. Can you take another look? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f6c18e2e9
ℹ️ 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".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 6f6c18e2e984ae6c11cc09d6673641c181ff4631 against 17d994bd3045dae71acda676a4ef4762ed8ecd94. No actionable findings remain.
The updated wording fixes the approval-lifetime issue: it distinguishes a reused RunContextWrapper from a plain application context and explains that overriding context while resuming RunState preserves approvals. The added ApplyPatchTool guidance and the distinction between session history and approval state also match the implementation.
Regarding the new CustomTool comment: its runtime premise is correct. The warning explicitly covers shell and file-edit tools, and the function-tool paragraph refers to FunctionTool, a separate type. I consider a CustomTool warning optional follow-up documentation.
Validation: both changed documents, the surrounding approval/context/serialization code and test source, plus an independent review. At this head, 20 hosted checks passed and 5 were skipped; the passing checks include the documentation build. This was a source-only review; I ran no candidate code, tests, or local documentation build.
This pull request updates the human-in-the-loop and tools guides to explain that persistent approvals and rejections for local
ShellToolinstances are shared by tool name across agent handoffs, including when executors have different permissions.The guidance recommends per-call decisions when agents have different shell permissions, explains that per-call decisions do not clear existing persistent decisions, and distinguishes agent-scoped function-tool approvals. This documentation describes existing behavior; it does not change the approval implementation.