Skip to content

Conversation

dylan-hurd-oai
Copy link
Collaborator

Summary

Follow up to #2186 for #2072 - we added handling for applypatch in default commands, but forgot to add detection to the heredocs logic.

Testing

  • Added unit tests

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

[P1] Allow heredoc extractor to accept applypatch

[lib.rs:264-268] maybe_parse_apply_patch now treats scripts starting with either apply_patch or applypatch as potential heredoc calls (lines 84‑96), but extract_heredoc_body_from_apply_patch_command still rejects any command not beginning with apply_patch (lines 264‑269). A bash command applypatch <<EOF will therefore yield ShellParseError instead of parsing the patch, defeating the intended fix.

Reply with @codex fix comments to fix any unresolved comments.

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 that is ready for review, or mark a draft as ready for review. You can also ask for a review by commenting "@codex review".

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

@dylan-hurd-oai dylan-hurd-oai added the codex-rust-review Perform a detailed review of Rust changes. label Aug 20, 2025
@dylan-hurd-oai dylan-hurd-oai requested a review from gpeal August 20, 2025 18:29
@github-actions github-actions bot added codex-rust-review-in-progress and removed codex-rust-review Perform a detailed review of Rust changes. labels Aug 20, 2025
Copy link

Summary
This PR broadens the apply-patch parser to accept both apply_patch and applypatch invocations and updates heredoc parsing accordingly. It also adds unit tests covering the new alias in both literal and heredoc forms.

  • codex-rs/apply-patch/src/lib.rs: adds top-level const APPLY_PATCH_COMMANDS: [&str; 2], replaces string checks with .iter().any(...), and updates tests (test_literal_applypatch, test_heredoc_applypatch).

Review
Looks good: the refactor centralizes command aliases and the tests cover both entry points in literal and heredoc modes.

  • codex-rs/apply-patch/src/lib.rs (APPLY_PATCH_COMMANDS addition): Nice extraction; used consistently in maybe_parse_apply_patch and extract_heredoc_body_from_apply_patch_command.
  • codex-rs/apply-patch/src/lib.rs (.any(|cmd| script.trim_start().starts_with(cmd))): Correctly generalizes the heredoc gate to support both aliases.
  • codex-rs/apply-patch/src/lib.rs (tests): Good coverage for both apply_patch and applypatch in literal and heredoc forms.
  • Nit: ExtractHeredocError::CommandDidNotStartWithApplyPatch name is now slightly misleading given the alias; consider renaming to a command-agnostic variant in a follow-up.
  • Optional: consider naming APPLY_PATCH_COMMANDS to APPLY_PATCH_ALIASES for clarity.

View workflow run

@dylan-hurd-oai dylan-hurd-oai merged commit 1a1516a into main Aug 20, 2025
16 checks passed
@dylan-hurd-oai dylan-hurd-oai deleted the dh--applypath-test branch August 20, 2025 19:16
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants