Skip to content

Commit 5cf2224

Browse files
iagooariagooar
committed
Fix flaky fake cli test (failed on CI)
1 parent 79a4480 commit 5cf2224

File tree

5 files changed

+149
-85
lines changed

5 files changed

+149
-85
lines changed

‎Cargo.toml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,6 @@ tokio = { version = "1.39", features = ["rt-multi-thread", "macros", "io-util"]
4242
serial_test = "3.1"
4343
assert_cmd = "2.0"
4444
rcgen = "0.13"
45+
46+
[target.'cfg(unix)'.dev-dependencies]
47+
libc = "0.2"

‎src/perms.rs‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,6 @@ pub fn set_custom_allowlist(commands: Vec<String>) {
5252
}
5353
}
5454

55-
pub fn clear_custom_allowlist() {
56-
custom_allowlist().lock().unwrap().clear();
57-
}
58-
5955
/// Ensure a path is within HOME or current working directory.
6056
pub fn ensure_safe_path(p: &Path) -> Result<()> {
6157
let resolved = resolve_path(p)?;

‎tests/cli_backend_tests.rs‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
#![cfg(unix)]
22

3+
use anyhow::Result;
34
use qqqa::ai::{CliCompletionRequest, run_cli_completion};
45
use qqqa::config::CliEngine;
56
use std::fs;
67
use std::os::unix::fs::PermissionsExt;
78
use std::path::Path;
89
use tempfile::tempdir;
10+
use tokio::time::{sleep, Duration};
911

1012
fn read_args(path: &Path) -> Vec<String> {
1113
let data = fs::read(path).expect("args file");
@@ -33,6 +35,39 @@ fn write_executable_script(path: &Path, contents: &str) {
3335
fs::set_permissions(path, perms).unwrap();
3436
}
3537

38+
async fn run_cli_completion_with_retry<'a, F>(mut make_req: F) -> Result<String>
39+
where
40+
F: FnMut() -> CliCompletionRequest<'a>,
41+
{
42+
const MAX_ATTEMPTS: usize = 3;
43+
let mut attempt = 0;
44+
loop {
45+
match run_cli_completion(make_req()).await {
46+
Ok(text) => return Ok(text),
47+
Err(err) => {
48+
if should_retry_etxtbsy(&err) && attempt < MAX_ATTEMPTS {
49+
attempt += 1;
50+
sleep(Duration::from_millis(25 * attempt as u64)).await;
51+
continue;
52+
}
53+
return Err(err);
54+
}
55+
}
56+
}
57+
}
58+
59+
#[cfg(unix)]
60+
fn should_retry_etxtbsy(err: &anyhow::Error) -> bool {
61+
err.chain()
62+
.filter_map(|cause| cause.downcast_ref::<std::io::Error>())
63+
.any(|io_err| io_err.raw_os_error() == Some(libc::ETXTBSY))
64+
}
65+
66+
#[cfg(not(unix))]
67+
fn should_retry_etxtbsy(_: &anyhow::Error) -> bool {
68+
false
69+
}
70+
3671
#[tokio::test]
3772
async fn run_cli_completion_returns_agent_message_from_script() {
3873
let dir = tempdir().unwrap();
@@ -43,7 +78,7 @@ printf '%s\n' '{"type":"item.completed","item":{"type":"agent_message","text":"h
4378
"#;
4479
write_executable_script(&script_path, script);
4580

46-
let text = run_cli_completion(CliCompletionRequest {
81+
let text = run_cli_completion_with_retry(|| CliCompletionRequest {
4782
engine: CliEngine::Codex,
4883
binary: script_path.to_str().unwrap(),
4984
base_args: &[],
@@ -79,7 +114,7 @@ printf '%s\n' '{{"type":"item.completed","item":{{"type":"agent_message","text":
79114
write_executable_script(&script_path, &script);
80115

81116
let base_args = vec!["exec".to_string()];
82-
let text = run_cli_completion(CliCompletionRequest {
117+
let text = run_cli_completion_with_retry(|| CliCompletionRequest {
83118
engine: CliEngine::Codex,
84119
binary: script_path.to_str().unwrap(),
85120
base_args: &base_args,

‎tests/perms_tests.rs‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
11
use qqqa::perms::{
2-
CommandDisposition, clear_custom_allowlist, ensure_safe_command, ensure_safe_path,
3-
set_custom_allowlist,
2+
CommandDisposition, ensure_safe_command, ensure_safe_path, set_custom_allowlist,
43
};
54
use serial_test::serial;
65
use std::path::Path;
76

87
#[test]
98
#[serial]
109
fn ensure_safe_command_allows_allowlisted_commands() {
11-
clear_custom_allowlist();
10+
set_custom_allowlist(Vec::new());
1211
unsafe {
1312
std::env::remove_var("QQQA_ALLOW_UNSAFE_COMMANDS");
1413
}
@@ -37,7 +36,7 @@ fn ensure_safe_command_allows_allowlisted_commands() {
3736
#[test]
3837
#[serial]
3938
fn ensure_safe_command_blocks_dangerous_patterns() {
40-
clear_custom_allowlist();
39+
set_custom_allowlist(Vec::new());
4140
unsafe {
4241
std::env::remove_var("QQQA_ALLOW_UNSAFE_COMMANDS");
4342
}
@@ -50,7 +49,7 @@ fn ensure_safe_command_blocks_dangerous_patterns() {
5049
#[test]
5150
#[serial]
5251
fn ensure_safe_command_blocks_connectors_and_inplace_edits() {
53-
clear_custom_allowlist();
52+
set_custom_allowlist(Vec::new());
5453
unsafe {
5554
std::env::remove_var("QQQA_ALLOW_UNSAFE_COMMANDS");
5655
}
@@ -73,7 +72,7 @@ fn ensure_safe_command_blocks_connectors_and_inplace_edits() {
7372
#[test]
7473
#[serial]
7574
fn ensure_safe_command_checks_all_segments() {
76-
clear_custom_allowlist();
75+
set_custom_allowlist(Vec::new());
7776
unsafe {
7877
std::env::remove_var("QQQA_ALLOW_UNSAFE_COMMANDS");
7978
}
@@ -89,7 +88,7 @@ fn ensure_safe_command_checks_all_segments() {
8988
#[test]
9089
#[serial]
9190
fn ensure_safe_command_respects_override_env() {
92-
clear_custom_allowlist();
91+
set_custom_allowlist(Vec::new());
9392
unsafe {
9493
std::env::set_var("QQQA_ALLOW_UNSAFE_COMMANDS", "1");
9594
}
@@ -105,7 +104,7 @@ fn ensure_safe_command_respects_override_env() {
105104
#[test]
106105
#[serial]
107106
fn ensure_safe_command_respects_custom_allowlist() {
108-
clear_custom_allowlist();
107+
set_custom_allowlist(Vec::new());
109108
unsafe {
110109
std::env::remove_var("QQQA_ALLOW_UNSAFE_COMMANDS");
111110
}
@@ -114,7 +113,7 @@ fn ensure_safe_command_respects_custom_allowlist() {
114113
ensure_safe_command("ffmpeg -i in.mp4 out.mp3").unwrap(),
115114
CommandDisposition::Allowed
116115
));
117-
clear_custom_allowlist();
116+
set_custom_allowlist(Vec::new());
118117
}
119118

120119
#[test]

‎tests/tools_tests.rs‎

Lines changed: 101 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use qqqa::perms::{clear_custom_allowlist, ensure_safe_path};
1+
use qqqa::perms::{ensure_safe_path, set_custom_allowlist};
22
use qqqa::shell::ShellKind;
33
use qqqa::tools::parse_tool_call;
44
use qqqa::tools::read_file;
@@ -15,7 +15,10 @@ struct TempCwdGuard {
1515
impl TempCwdGuard {
1616
fn new(dir: &Path) -> Self {
1717
static CWD_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
18-
let lock = CWD_LOCK.get_or_init(|| Mutex::new(())).lock().unwrap();
18+
let lock = CWD_LOCK
19+
.get_or_init(|| Mutex::new(()))
20+
.lock()
21+
.unwrap_or_else(|poisoned| poisoned.into_inner());
1922
let previous = std::env::current_dir().expect("read current dir");
2023
std::env::set_current_dir(dir).expect("set current dir");
2124
Self {
@@ -31,12 +34,47 @@ impl Drop for TempCwdGuard {
3134
}
3235
}
3336

37+
struct EnvVarGuard {
38+
key: &'static str,
39+
previous: Option<String>,
40+
}
41+
42+
impl EnvVarGuard {
43+
fn set(key: &'static str, value: Option<&str>) -> Self {
44+
let previous = std::env::var(key).ok();
45+
if let Some(val) = value {
46+
unsafe {
47+
std::env::set_var(key, val);
48+
}
49+
} else {
50+
unsafe {
51+
std::env::remove_var(key);
52+
}
53+
}
54+
Self { key, previous }
55+
}
56+
}
57+
58+
impl Drop for EnvVarGuard {
59+
fn drop(&mut self) {
60+
if let Some(ref val) = self.previous {
61+
unsafe {
62+
std::env::set_var(self.key, val);
63+
}
64+
} else {
65+
unsafe {
66+
std::env::remove_var(self.key);
67+
}
68+
}
69+
}
70+
}
71+
3472
#[test]
3573
#[serial]
3674
fn read_and_write_file_tools_and_path_safety() {
3775
// Set HOME and CWD to a temp folder for deterministic safety checks.
3876
let temp = tempfile::tempdir().unwrap();
39-
clear_custom_allowlist();
77+
set_custom_allowlist(Vec::new());
4078
unsafe {
4179
std::env::set_var("HOME", temp.path());
4280
}
@@ -141,39 +179,38 @@ async fn execute_command_honors_pty_force_flag() {
141179
let _cwd_guard = TempCwdGuard::new(temp.path());
142180

143181
let probe = "env sh -lc '[ -t 1 ] && echo tty || echo notty'";
144-
let baseline = qqqa::tools::execute_command::run(
145-
qqqa::tools::execute_command::Args {
146-
command: probe.into(),
147-
cwd: None,
148-
},
149-
true,
150-
false,
151-
ShellKind::Posix,
152-
None,
153-
)
154-
.await
155-
.expect("baseline execute_command should succeed");
182+
let baseline = {
183+
let _disable_guard = EnvVarGuard::set("QQQA_DISABLE_PTY", Some("1"));
184+
qqqa::tools::execute_command::run(
185+
qqqa::tools::execute_command::Args {
186+
command: probe.into(),
187+
cwd: None,
188+
},
189+
true,
190+
false,
191+
ShellKind::Posix,
192+
None,
193+
)
194+
.await
195+
.expect("baseline execute_command should succeed")
196+
};
156197
assert!(baseline.contains("notty"));
157198

158-
unsafe {
159-
std::env::set_var("QQQA_FORCE_PTY", "1");
160-
}
161-
let forced = qqqa::tools::execute_command::run(
162-
qqqa::tools::execute_command::Args {
163-
command: probe.into(),
164-
cwd: None,
165-
},
166-
true,
167-
false,
168-
ShellKind::Posix,
169-
None,
170-
)
171-
.await
172-
.expect("execute_command with forced PTY should succeed");
173-
174-
unsafe {
175-
std::env::remove_var("QQQA_FORCE_PTY");
176-
}
199+
let forced = {
200+
let _force_guard = EnvVarGuard::set("QQQA_FORCE_PTY", Some("1"));
201+
qqqa::tools::execute_command::run(
202+
qqqa::tools::execute_command::Args {
203+
command: probe.into(),
204+
cwd: None,
205+
},
206+
true,
207+
false,
208+
ShellKind::Posix,
209+
None,
210+
)
211+
.await
212+
.expect("execute_command with forced PTY should succeed")
213+
};
177214

178215
assert!(
179216
forced.contains("tty"),
@@ -193,43 +230,37 @@ async fn execute_command_respects_disable_flag() {
193230
let _cwd_guard = TempCwdGuard::new(temp.path());
194231

195232
let probe = "env sh -lc '[ -t 1 ] && echo tty || echo notty'";
196-
unsafe {
197-
std::env::set_var("QQQA_FORCE_PTY", "1");
198-
}
199-
let forced = qqqa::tools::execute_command::run(
200-
qqqa::tools::execute_command::Args {
201-
command: probe.into(),
202-
cwd: None,
203-
},
204-
true,
205-
false,
206-
ShellKind::Posix,
207-
None,
208-
)
209-
.await
210-
.expect("forced PTY should succeed");
211-
212-
unsafe {
213-
std::env::remove_var("QQQA_FORCE_PTY");
214-
std::env::set_var("QQQA_DISABLE_PTY", "1");
215-
}
216-
217-
let res = qqqa::tools::execute_command::run(
218-
qqqa::tools::execute_command::Args {
219-
command: probe.into(),
220-
cwd: None,
221-
},
222-
true,
223-
false,
224-
ShellKind::Posix,
225-
None,
226-
)
227-
.await
228-
.expect("execute_command fallback should succeed");
233+
let forced = {
234+
let _force_guard = EnvVarGuard::set("QQQA_FORCE_PTY", Some("1"));
235+
qqqa::tools::execute_command::run(
236+
qqqa::tools::execute_command::Args {
237+
command: probe.into(),
238+
cwd: None,
239+
},
240+
true,
241+
false,
242+
ShellKind::Posix,
243+
None,
244+
)
245+
.await
246+
.expect("forced PTY should succeed")
247+
};
229248

230-
unsafe {
231-
std::env::remove_var("QQQA_DISABLE_PTY");
232-
}
249+
let res = {
250+
let _disable_guard = EnvVarGuard::set("QQQA_DISABLE_PTY", Some("1"));
251+
qqqa::tools::execute_command::run(
252+
qqqa::tools::execute_command::Args {
253+
command: probe.into(),
254+
cwd: None,
255+
},
256+
true,
257+
false,
258+
ShellKind::Posix,
259+
None,
260+
)
261+
.await
262+
.expect("execute_command fallback should succeed")
263+
};
233264

234265
assert!(forced.contains("tty"));
235266
assert!(

0 commit comments

Comments
 (0)