Skip to content

feat: enhance Termux compatibility with centralized utility module #1719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/rust-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ jobs:
- name: cargo clippy
id: clippy
continue-on-error: true
run: cargo clippy --target ${{ matrix.target }} --all-features --tests -- -D warnings
run: |
cargo clippy --target ${{ matrix.target }} --all-features --tests -- -D warnings \
-A clippy::collapsible_if \
-A clippy::cloned_ref_to_slice_refs

# Running `cargo build` from the workspace root builds the workspace using
# the union of all features from third-party crates. This can mask errors
Expand Down
36 changes: 18 additions & 18 deletions codex-rs/apply-patch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,12 +357,12 @@ fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result<AffectedPaths> {
for hunk in hunks {
match hunk {
Hunk::AddFile { path, contents } => {
if let Some(parent) = path.parent() {
if !parent.as_os_str().is_empty() {
std::fs::create_dir_all(parent).with_context(|| {
format!("Failed to create parent directories for {}", path.display())
})?;
}
if let Some(parent) = path.parent()
&& !parent.as_os_str().is_empty()
{
std::fs::create_dir_all(parent).with_context(|| {
format!("Failed to create parent directories for {}", path.display())
})?;
}
std::fs::write(path, contents)
.with_context(|| format!("Failed to write file {}", path.display()))?;
Expand All @@ -381,15 +381,12 @@ fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result<AffectedPaths> {
let AppliedPatch { new_contents, .. } =
derive_new_contents_from_chunks(path, chunks)?;
if let Some(dest) = move_path {
if let Some(parent) = dest.parent() {
if !parent.as_os_str().is_empty() {
std::fs::create_dir_all(parent).with_context(|| {
format!(
"Failed to create parent directories for {}",
dest.display()
)
})?;
}
if let Some(parent) = dest.parent()
&& !parent.as_os_str().is_empty()
{
std::fs::create_dir_all(parent).with_context(|| {
format!("Failed to create parent directories for {}", dest.display())
})?;
}
std::fs::write(dest, new_contents)
.with_context(|| format!("Failed to write file {}", dest.display()))?;
Expand Down Expand Up @@ -471,9 +468,12 @@ fn compute_replacements(
// If a chunk has a `change_context`, we use seek_sequence to find it, then
// adjust our `line_index` to continue from there.
if let Some(ctx_line) = &chunk.change_context {
if let Some(idx) =
seek_sequence::seek_sequence(original_lines, &[ctx_line.clone()], line_index, false)
{
if let Some(idx) = seek_sequence::seek_sequence(
original_lines,
std::slice::from_ref(ctx_line),
line_index,
false,
) {
line_index = idx + 1;
} else {
return Err(ApplyPatchError::ComputeReplacements(format!(
Expand Down
157 changes: 77 additions & 80 deletions codex-rs/core/src/apply_patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,74 +127,71 @@ pub(crate) async fn apply_patch(
// and prompt the user to extend permissions.
let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr);

if let Err(err) = &result {
if err.kind() == std::io::ErrorKind::PermissionDenied {
// Determine first offending path.
let offending_opt = action
.changes()
.iter()
.flat_map(|(path, change)| match change {
ApplyPatchFileChange::Add { .. } => vec![path.as_ref()],
ApplyPatchFileChange::Delete => vec![path.as_ref()],
ApplyPatchFileChange::Update {
move_path: Some(move_path),
..
} => {
vec![path.as_ref(), move_path.as_ref()]
}
ApplyPatchFileChange::Update {
move_path: None, ..
} => vec![path.as_ref()],
})
.find_map(|path: &Path| {
// ApplyPatchAction promises to guarantee absolute paths.
if !path.is_absolute() {
panic!("apply_patch invariant failed: path is not absolute: {path:?}");
}

let writable = {
#[allow(clippy::unwrap_used)]
let roots = sess.writable_roots.lock().unwrap();
roots.iter().any(|root| path.starts_with(root))
};
if writable {
None
} else {
Some(path.to_path_buf())
}
});

if let Some(offending) = offending_opt {
let root = offending.parent().unwrap_or(&offending).to_path_buf();
if let Err(err) = &result
&& err.kind() == std::io::ErrorKind::PermissionDenied
{
// Determine first offending path.
let offending_opt = action
.changes()
.iter()
.flat_map(|(path, change)| match change {
ApplyPatchFileChange::Add { .. } => vec![path.as_ref()],
ApplyPatchFileChange::Delete => vec![path.as_ref()],
ApplyPatchFileChange::Update {
move_path: Some(move_path),
..
} => {
vec![path.as_ref(), move_path.as_ref()]
}
ApplyPatchFileChange::Update {
move_path: None, ..
} => vec![path.as_ref()],
})
.find_map(|path: &Path| {
// ApplyPatchAction promises to guarantee absolute paths.
if !path.is_absolute() {
panic!("apply_patch invariant failed: path is not absolute: {path:?}");
}

let reason = Some(format!(
"grant write access to {} for this session",
root.display()
));
let rx = sess
.request_patch_approval(
sub_id.clone(),
call_id.clone(),
&action,
reason.clone(),
Some(root.clone()),
)
.await;
if matches!(
rx.await.unwrap_or_default(),
ReviewDecision::Approved | ReviewDecision::ApprovedForSession
) {
// Extend writable roots.
let writable = {
#[allow(clippy::unwrap_used)]
sess.writable_roots.lock().unwrap().push(root);
stdout.clear();
stderr.clear();
result = apply_changes_from_apply_patch_and_report(
&action,
&mut stdout,
&mut stderr,
);
let roots = sess.writable_roots.lock().unwrap();
roots.iter().any(|root| path.starts_with(root))
};
if writable {
None
} else {
Some(path.to_path_buf())
}
});

if let Some(offending) = offending_opt {
let root = offending.parent().unwrap_or(&offending).to_path_buf();

let reason = Some(format!(
"grant write access to {} for this session",
root.display()
));
let rx = sess
.request_patch_approval(
sub_id.clone(),
call_id.clone(),
&action,
reason.clone(),
Some(root.clone()),
)
.await;
if matches!(
rx.await.unwrap_or_default(),
ReviewDecision::Approved | ReviewDecision::ApprovedForSession
) {
// Extend writable roots.
#[allow(clippy::unwrap_used)]
sess.writable_roots.lock().unwrap().push(root);
stdout.clear();
stderr.clear();
result =
apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr);
}
}
}
Expand Down Expand Up @@ -325,12 +322,12 @@ fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result<A
for (path, change) in changes {
match change {
ApplyPatchFileChange::Add { content } => {
if let Some(parent) = path.parent() {
if !parent.as_os_str().is_empty() {
std::fs::create_dir_all(parent).with_context(|| {
format!("Failed to create parent directories for {}", path.display())
})?;
}
if let Some(parent) = path.parent()
&& !parent.as_os_str().is_empty()
{
std::fs::create_dir_all(parent).with_context(|| {
format!("Failed to create parent directories for {}", path.display())
})?;
}
std::fs::write(path, content)
.with_context(|| format!("Failed to write file {}", path.display()))?;
Expand All @@ -347,15 +344,15 @@ fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result<A
new_content,
} => {
if let Some(move_path) = move_path {
if let Some(parent) = move_path.parent() {
if !parent.as_os_str().is_empty() {
std::fs::create_dir_all(parent).with_context(|| {
format!(
"Failed to create parent directories for {}",
move_path.display()
)
})?;
}
if let Some(parent) = move_path.parent()
&& !parent.as_os_str().is_empty()
{
std::fs::create_dir_all(parent).with_context(|| {
format!(
"Failed to create parent directories for {}",
move_path.display()
)
})?;
}

std::fs::rename(path, move_path)
Expand Down
42 changes: 20 additions & 22 deletions codex-rs/core/src/chat_completions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,27 +272,25 @@ async fn process_chat_sse<S>(
.get("delta")
.and_then(|d| d.get("tool_calls"))
.and_then(|tc| tc.as_array())
&& let Some(tool_call) = tool_calls.first()
{
if let Some(tool_call) = tool_calls.first() {
// Mark that we have an active function call in progress.
fn_call_state.active = true;
// Mark that we have an active function call in progress.
fn_call_state.active = true;

// Extract call_id if present.
if let Some(id) = tool_call.get("id").and_then(|v| v.as_str()) {
fn_call_state.call_id.get_or_insert_with(|| id.to_string());
}
// Extract call_id if present.
if let Some(id) = tool_call.get("id").and_then(|v| v.as_str()) {
fn_call_state.call_id.get_or_insert_with(|| id.to_string());
}

// Extract function details if present.
if let Some(function) = tool_call.get("function") {
if let Some(name) = function.get("name").and_then(|n| n.as_str()) {
fn_call_state.name.get_or_insert_with(|| name.to_string());
}
// Extract function details if present.
if let Some(function) = tool_call.get("function") {
if let Some(name) = function.get("name").and_then(|n| n.as_str()) {
fn_call_state.name.get_or_insert_with(|| name.to_string());
}

if let Some(args_fragment) =
function.get("arguments").and_then(|a| a.as_str())
{
fn_call_state.arguments.push_str(args_fragment);
}
if let Some(args_fragment) = function.get("arguments").and_then(|a| a.as_str())
{
fn_call_state.arguments.push_str(args_fragment);
}
}
}
Expand Down Expand Up @@ -384,13 +382,13 @@ where
let is_assistant_delta = matches!(&item, crate::models::ResponseItem::Message { role, .. } if role == "assistant");

if is_assistant_delta {
if let crate::models::ResponseItem::Message { content, .. } = &item {
if let Some(text) = content.iter().find_map(|c| match c {
if let crate::models::ResponseItem::Message { content, .. } = &item
&& let Some(text) = content.iter().find_map(|c| match c {
crate::models::ContentItem::OutputText { text } => Some(text),
_ => None,
}) {
this.cumulative.push_str(text);
}
})
{
this.cumulative.push_str(text);
}

// Swallow partial assistant chunk; keep polling.
Expand Down
42 changes: 21 additions & 21 deletions codex-rs/core/src/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,10 @@ impl Session {

pub fn remove_task(&self, sub_id: &str) {
let mut state = self.state.lock().unwrap();
if let Some(task) = &state.current_task {
if task.sub_id == sub_id {
state.current_task.take();
}
if let Some(task) = &state.current_task
&& task.sub_id == sub_id
{
state.current_task.take();
}
}

Expand Down Expand Up @@ -706,11 +706,11 @@ async fn submission_loop(
}));

// Patch restored state into the newly created session.
if let Some(sess_arc) = &sess {
if restored_items.is_some() {
let mut st = sess_arc.state.lock().unwrap();
st.history.record_items(restored_items.unwrap().iter());
}
if let Some(sess_arc) = &sess
&& restored_items.is_some()
{
let mut st = sess_arc.state.lock().unwrap();
st.history.record_items(restored_items.unwrap().iter());
}

// Gather history metadata for SessionConfiguredEvent.
Expand Down Expand Up @@ -829,18 +829,18 @@ async fn submission_loop(
// that inspect the rollout file do not race with the background writer.
if let Some(sess_arc) = sess {
let recorder_opt = sess_arc.rollout.lock().unwrap().take();
if let Some(rec) = recorder_opt {
if let Err(e) = rec.shutdown().await {
warn!("failed to shutdown rollout recorder: {e}");
let event = Event {
id: sub.id.clone(),
msg: EventMsg::Error(ErrorEvent {
message: "Failed to shutdown rollout recorder".to_string(),
}),
};
if let Err(e) = tx_event.send(event).await {
warn!("failed to send error message: {e:?}");
}
if let Some(rec) = recorder_opt
&& let Err(e) = rec.shutdown().await
{
warn!("failed to shutdown rollout recorder: {e}");
let event = Event {
id: sub.id.clone(),
msg: EventMsg::Error(ErrorEvent {
message: "Failed to shutdown rollout recorder".to_string(),
}),
};
if let Err(e) = tx_event.send(event).await {
warn!("failed to send error message: {e:?}");
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions codex-rs/core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,10 +607,10 @@ fn default_model() -> String {
pub fn find_codex_home() -> std::io::Result<PathBuf> {
// Honor the `CODEX_HOME` environment variable when it is set to allow users
// (and tests) to override the default location.
if let Ok(val) = std::env::var("CODEX_HOME") {
if !val.is_empty() {
return PathBuf::from(val).canonicalize();
}
if let Ok(val) = std::env::var("CODEX_HOME")
&& !val.is_empty()
{
return PathBuf::from(val).canonicalize();
}

let mut p = home_dir().ok_or_else(|| {
Expand Down
Loading
Loading