Repository navigation
feat: add structured project search to native environments - #5574
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (3)
ℹ️ 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.
The existing reserved-name finding is valid: application FunctionTools named project_glob/project_grep are rejected even without opting into native search. Please condition collision checks on enabled native tools and retain the existing no-Environment path. Referencing the existing discussion rather than duplicating it: #5574 (comment)
Coordinator and independent static review covered rooted filesystem access, bounded traversal/read/output, executor capabilities, result policies, restore, cancellation, and relevant regression tests. No additional actionable findings. Hosted native-platform CI and Rust CodeQL remain pending. No candidate code or tests executed.
![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: cb9f9553a6
ℹ️ 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.
The application tool-name compatibility issue is fixed: names remain available unless the matching Environment tool is enabled. Static re-review confirms one remaining P2 already reported at #5574 (comment): a single non-UTF-8 directory entry aborts read_directory_bounded and causes sync_walk_no_follow to discard every valid sibling and subdirectory. Retain valid entries while reporting/skipping the unsupported name, keeping skipped entries within the enumeration budget. No duplicate inline finding added.
Coordinator and independent static review only; no candidate code or local tests run. Current-head hosted platform CI remains pending. Merge/release hold remains in place.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The remaining filename finding is fixed at d73996b. Unsupported Unix/Windows names are individual errors, so valid siblings and descendants survive. Errors consume the scan budget before being skipped; enumeration failures preserve prior entries, while setup/cancellation failures retain their prior behavior. Native and public regression coverage addresses these cases; patch SHA-256 matches the manifest.
Coordinator and independent static re-review found no additional actionable issue. No candidate code or tests executed locally. Current-head platform CI and CodeQL remain pending; Windows-specific native cases are author-reported unexecuted. Keep the stated merge/release hold until validation is complete.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: d73996bf12
ℹ️ 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.
Changing the earlier approval to request changes for the newly identified P2 path-authorization mismatch, independently validated against the exact pinned source: #5574 (comment)
The pre-dispatch Environment callback receives the original arguments. relative_path accepts TAB/LF/CR, but root.join uses pinned PathUri::join -> url 2.5.8 PathSegmentsMut -> parser Input, which drops those characters. For example, a callback denying the secrets subtree can approve the distinct spelling sec\nrets, after which project_grep walks and reads secrets. Root containment and no-follow traversal do not prevent this within-root policy bypass.
Reject these characters before filesystem dispatch, or preserve path spelling losslessly and ensure authorization applies to the actual I/O path. Add regression coverage for TAB/LF/CR with a subtree-denying callback and verify denied content is never read or returned. Referencing the existing discussion rather than duplicating an inline finding.
Coordinator and independent static source assessment; no candidate code/tests executed. The previous filename fix remains valid. Hosted CI and merge/release follow-up remain open.
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: 81f5f267ac
ℹ️ 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 control-character path fix, native filename identity fix, and EILSEQ-only fixture gating address the prior findings. One authorization-normalization gap remains: ./secrets passes relative_path and the existing regression callback (which denies the first component secrets), then root.join normalizes it to secrets before filesystem access. I independently verified the existing finding: #5574 (comment) . Please reject dot components in non-root paths (preserving the complete default path .), or authorize the same canonical path used for I/O; cover both tools with a denying callback and no protected filesystem access. Referencing the existing thread instead of duplicating its inline finding.
Coordinator and independent static re-review of this exact commit; no candidate code or tests executed. Hosted Linux/macOS/Windows native snapshot jobs and Rust CodeQL remain pending.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The remaining path-authorization blocker is fixed: non-root paths reject dot components, and the joined path must round-trip to the exact approved relative spelling before any filesystem access. The whole-path root sentinel remains supported. Reviewed both-tool denial regressions, literal percent/Unicode/space round trips, and the drive-like URI edge case against the pinned path implementation. No new actionable findings.
Coordinator and independent static re-review; no candidate code or tests executed. Hosted Linux/macOS/Windows native snapshots and Rust CodeQL remain pending. This supersedes my prior request-changes decision; CI follow-up remains open.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: ae9a9c01dd
ℹ️ 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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
🛡️ 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. |
|
@dpiet-oai Please re-review |
Summary
This pull request adds opt-in
project_globandproject_greptools to nativeEnvironmentsessions. Agents can find files and matching lines as structured results without writing shell commands. Search stays within the currentEnvironment.cwdand returns relative paths, line context, errors, and whether limits cut the search short.Search uses the existing approval, argument, stop, and result policies. It does not follow symlinks and caps directory scans, file reads, and output size. Remote executors must advertise support for these bounds. Search rejects control characters and dot components in requested paths, except for the
.root. It also checks that joining a path to the root preserves the exact spelling approved by policy. Traversal preserves literal filenames. Restored sessions use the current root and tool grants after executor-policy checks pass. Application tools may use either search name when the corresponding native tool is disabled. The pinned native revision stays unchanged.Test plan
ae9a9c01ddded54fde57a325860558e22d19317b: 24 checks passed and 15 were skipped. Current code and security reviews are complete, with all review threads resolved. The earlier changes-requested maintainer review still needs to be updated.Issue number
None linked.
Checks
AGENTS.md, including.agents/skills/code-change-verification/scripts/run.shwhen requiredAGENTS.md