mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Reject unsupported options for codex queue (#57273)
## What changed - Validate inherited options before and after the `queue` subcommand, with explicit errors for unsupported flags and positional prompts. - Reject `--model` and `-c model=...` because queued messages use the thread's model. - Allow `--cd` for local server startup, but reject it with `--remote`. - Hide unsupported inherited options from queue help and clarify the scope of startup configuration overrides. ## Testing Add regression coverage for unsupported options in both argument scopes, model configuration overrides, remote `--cd` conflicts, and queue help output. Preserve coverage for overrides that bypass a running local daemon. GitOrigin-RevId: 703ba26f38930a01559847e66e1af3d0d338d1e9
This commit is contained in:
@@ -3,7 +3,7 @@
|
||||
use crate::MultitoolCli;
|
||||
use clap::Command;
|
||||
use clap::CommandFactory;
|
||||
use clap::Parser;
|
||||
use clap::FromArgMatches;
|
||||
use codex_utils_cli::CliConfigOverrides;
|
||||
use std::ffi::OsString;
|
||||
|
||||
@@ -11,7 +11,11 @@ pub(super) fn try_parse_from(
|
||||
args: impl IntoIterator<Item = impl Into<OsString>>,
|
||||
) -> Result<MultitoolCli, clap::Error> {
|
||||
let args: Vec<OsString> = args.into_iter().map(Into::into).collect();
|
||||
let mut cli = MultitoolCli::try_parse_from(&args)?;
|
||||
let mut command = MultitoolCli::command();
|
||||
let mut matches = command.try_get_matches_from_mut(&args)?;
|
||||
crate::queue_cmd::validate_options(&command, &matches)?;
|
||||
let mut cli = MultitoolCli::from_arg_matches_mut(&mut matches)
|
||||
.map_err(|error| error.format(&mut command))?;
|
||||
if cli.subcommand.is_none() || cli.config_overrides.raw_overrides.is_empty() {
|
||||
return Ok(cli);
|
||||
}
|
||||
|
||||
@@ -3561,6 +3561,23 @@ enabled = true
|
||||
err.to_string()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn queue_help_lists_supported_options() {
|
||||
let help = help_from_args(&["codex", "queue", "--help"]);
|
||||
let (general, local) = help
|
||||
.split_once("Local server options (without --remote):")
|
||||
.unwrap();
|
||||
assert!(general.contains("--message <TEXT>"));
|
||||
assert!(!general.contains("--model"));
|
||||
assert!(!general.contains("-c model="));
|
||||
let local_flags = local
|
||||
.lines()
|
||||
.filter(|line| line.starts_with(" -") || line.starts_with(" --"))
|
||||
.map(str::trim)
|
||||
.collect::<Vec<_>>();
|
||||
insta::assert_snapshot!(local_flags.join("\n"), @"-C, --cd <DIR>");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn tcp_tunnel_is_hidden_and_accepts_its_control_input_contract() {
|
||||
let root_help = help_from_args(&["codex", "--help"]);
|
||||
|
||||
@@ -1,5 +1,11 @@
|
||||
use clap::Arg;
|
||||
use clap::ArgMatches;
|
||||
use clap::Command;
|
||||
use clap::CommandFactory;
|
||||
use clap::Parser;
|
||||
use clap::builder::NonEmptyStringValueParser;
|
||||
use clap::error::ErrorKind;
|
||||
use clap::parser::ValueSource;
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_tui::Cli as TuiCli;
|
||||
use codex_utils_cli::CliConfigOverrides;
|
||||
@@ -10,6 +16,7 @@ use crate::finalize_session_archive_interactive;
|
||||
use crate::resolve_remote_endpoint;
|
||||
|
||||
#[derive(Debug, Parser)]
|
||||
#[command(defer(configure_help))]
|
||||
pub(crate) struct QueueCommand {
|
||||
/// Session UUID or exact session name.
|
||||
#[arg(long, value_name = "THREAD")]
|
||||
@@ -31,6 +38,76 @@ pub(crate) struct QueueCommand {
|
||||
pub(crate) config_overrides: SessionArchiveConfigOverrides,
|
||||
}
|
||||
|
||||
/// Only inherited options need restrictions; queue owns its other arguments.
|
||||
fn restricted_options(command: &Command) -> impl Iterator<Item = &Arg> {
|
||||
command.get_arguments().filter(|arg| {
|
||||
!matches!(
|
||||
arg.get_long(),
|
||||
Some(
|
||||
"remote"
|
||||
| "remote-auth-token-env"
|
||||
| "config"
|
||||
| "enable"
|
||||
| "disable"
|
||||
| "profile"
|
||||
| "strict-config"
|
||||
)
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
fn configure_help(command: Command) -> Command {
|
||||
let interactive = TuiCli::command();
|
||||
command.mut_args(|arg| {
|
||||
if arg.get_long() == Some("config") {
|
||||
arg.long_help(None).help("Override a startup configuration value (key=value). Queued messages use the thread's model.")
|
||||
} else if !restricted_options(&interactive).any(|shared| shared.get_id() == arg.get_id()) {
|
||||
arg
|
||||
} else if arg.get_long() == Some("cd") {
|
||||
arg.help_heading("Local server options (without --remote)")
|
||||
} else {
|
||||
arg.hide(true)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
/// Check both scopes before root flags such as `--approve-for-me` become config overrides.
|
||||
pub(super) fn validate_options(command: &Command, root: &ArgMatches) -> Result<(), clap::Error> {
|
||||
let Some(queue) = root.subcommand_matches("queue") else {
|
||||
return Ok(());
|
||||
};
|
||||
let remote = root
|
||||
.get_one::<String>("remote")
|
||||
.or_else(|| queue.get_one::<String>("remote"));
|
||||
let supplied = [root, queue].into_iter().flat_map(|scope| {
|
||||
scope
|
||||
.ids()
|
||||
.filter(|id| scope.value_source(id.as_str()) == Some(ValueSource::CommandLine))
|
||||
});
|
||||
for id in supplied {
|
||||
let Some(arg) = restricted_options(command).find(|arg| arg.get_id() == id) else {
|
||||
continue;
|
||||
};
|
||||
let message = match arg.get_long() {
|
||||
Some("cd") if remote.is_none() => continue,
|
||||
Some("cd") => "`codex queue` does not support `--cd` with `--remote`".to_string(),
|
||||
// These flags already have dedicated diagnostics in the CLI and session startup.
|
||||
Some("worktree" | "no-daemon") => continue,
|
||||
Some("model") => {
|
||||
"`codex queue` does not support `--model`; queued messages use the thread's model"
|
||||
.to_string()
|
||||
}
|
||||
Some("image") => "`codex queue` does not support image attachments".to_string(),
|
||||
Some(name) => format!("`codex queue` does not support `--{name}`"),
|
||||
None => {
|
||||
"`codex queue` does not accept a positional prompt; use `--message`".to_string()
|
||||
}
|
||||
};
|
||||
return Err(clap::Error::raw(ErrorKind::ArgumentConflict, message));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn run_queue_command(
|
||||
command: QueueCommand,
|
||||
interactive: TuiCli,
|
||||
@@ -48,8 +125,14 @@ pub(crate) async fn run_queue_command(
|
||||
} = command;
|
||||
let cli =
|
||||
finalize_session_archive_interactive(interactive, root_config_overrides, config_overrides);
|
||||
if !cli.images.is_empty() {
|
||||
anyhow::bail!("`codex queue` does not support image attachments");
|
||||
if cli.config_overrides.raw_overrides.iter().any(|entry| {
|
||||
entry
|
||||
.split_once('=')
|
||||
.is_some_and(|(key, _)| key.trim() == "model")
|
||||
}) {
|
||||
anyhow::bail!(
|
||||
"`codex queue` does not support a `model` config override; queued messages use the thread's model"
|
||||
);
|
||||
}
|
||||
let explicit_remote_endpoint = resolve_remote_endpoint(
|
||||
remote.remote.or(root_remote),
|
||||
|
||||
+37
-17
@@ -182,20 +182,39 @@ fn queue_rejects_empty_message() -> Result<()> {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn queue_rejects_image_attachments() -> Result<()> {
|
||||
let output = std::process::Command::new(codex_utils_cargo_bin::cargo_bin("codex")?)
|
||||
.args([
|
||||
"queue",
|
||||
"--thread",
|
||||
THREAD_ID,
|
||||
"--message",
|
||||
"do the thing",
|
||||
"--image",
|
||||
"screenshot.png",
|
||||
])
|
||||
.output()?;
|
||||
assert!(!output.status.success());
|
||||
assert!(String::from_utf8(output.stderr)?.contains("does not support image attachments"));
|
||||
fn queue_rejects_unsupported_options() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
for (args, expected) in [
|
||||
("queue --image screenshot.png", "image attachments"),
|
||||
("queue --model other-model", "`--model`"),
|
||||
("--oss queue", "`--oss`"),
|
||||
("queue --local-provider ollama", "`--local-provider`"),
|
||||
(
|
||||
"queue --dangerously-bypass-hook-trust",
|
||||
"`--dangerously-bypass-hook-trust`",
|
||||
),
|
||||
(
|
||||
"-c model=other queue -c analytics.enabled=false",
|
||||
"`model` config override",
|
||||
),
|
||||
("queue -s read-only", "`--sandbox`"),
|
||||
("--approve-for-me queue", "`--approve-for-me`"),
|
||||
("prompt queue", "positional prompt; use `--message`"),
|
||||
("-C . queue --remote unix://", "`--cd` with `--remote`"),
|
||||
("--remote unix:// queue -C .", "`--cd` with `--remote`"),
|
||||
] {
|
||||
let output = std::process::Command::new(codex_utils_cargo_bin::cargo_bin("codex")?)
|
||||
.env("CODEX_HOME", codex_home.path())
|
||||
.args(args.split_whitespace())
|
||||
.args(["--thread", THREAD_ID, "--message", "do the thing"])
|
||||
.output()?;
|
||||
assert!(!output.status.success());
|
||||
let stderr = String::from_utf8(output.stderr)?;
|
||||
assert!(
|
||||
stderr.contains("`codex queue`") && stderr.contains(expected),
|
||||
"{args:?}: {stderr}"
|
||||
);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -631,8 +650,10 @@ async fn queue_rejects_overrides_that_bypass_local_daemon() -> Result<()> {
|
||||
.env("CODEX_HOME", codex_home.path())
|
||||
.args([
|
||||
"queue",
|
||||
"-C",
|
||||
".",
|
||||
"-c",
|
||||
"model=\"test-model\"",
|
||||
"analytics.enabled=false",
|
||||
"--thread",
|
||||
THREAD_ID,
|
||||
"--message",
|
||||
@@ -640,12 +661,11 @@ async fn queue_rejects_overrides_that_bypass_local_daemon() -> Result<()> {
|
||||
])
|
||||
.output()
|
||||
.await?;
|
||||
server.await??;
|
||||
|
||||
assert!(!output.status.success());
|
||||
assert!(
|
||||
String::from_utf8(output.stderr)?
|
||||
.contains("embedded app server while a local app-server daemon is running")
|
||||
);
|
||||
server.await??;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user