mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Respect hook trust bypass during TUI startup (#24317)
Fixes #24093. ## Why `--dangerously-bypass-hook-trust` is a supported CLI flag intended for headless or automated runs where enabled hooks should be allowed to run without requiring persisted trust. In the TUI, startup hook review still opened whenever hooks looked untrusted, so a launch using the bypass could block on the interactive "Hooks need review" prompt. The tricky case is persistent app-server resume: a resume may attach to an already-running thread, where resume config overrides are ignored. In that path, hiding the startup review would be wrong because the existing hook engine may still filter untrusted hooks. ## What Changed - Startup hook review now skips the prompt only when hook trust bypass is actually safe for that launch. - The TUI forwards `bypass_hook_trust` through the app-server request config for fresh thread start/resume/fork paths, and the app-server applies it as a runtime-only `ConfigOverrides` value rather than treating it like a `config.toml` setting. - Persistent app-server resumes keep the startup review prompt so users still have a chance to trust hooks when the running thread cannot receive the bypass override. ## Verification - Added focused coverage for startup hook review with and without `bypass_hook_trust`. - Extended existing TUI/app-server config override tests to cover forwarding and applying `bypass_hook_trust`.
This commit is contained in:
committed by
GitHub
Unverified
parent
913270a689
commit
5fb5e47767
@@ -216,15 +216,23 @@ impl ConfigManager {
|
||||
&self,
|
||||
cli_overrides: &[(String, TomlValue)],
|
||||
request_overrides: Option<HashMap<String, serde_json::Value>>,
|
||||
typesafe_overrides: ConfigOverrides,
|
||||
mut typesafe_overrides: ConfigOverrides,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> std::io::Result<Config> {
|
||||
let mut request_overrides = request_overrides.unwrap_or_default();
|
||||
if let Some(value) = request_overrides.remove("bypass_hook_trust") {
|
||||
typesafe_overrides.bypass_hook_trust = Some(value.as_bool().ok_or_else(|| {
|
||||
std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
"`bypass_hook_trust` override must be a boolean",
|
||||
)
|
||||
})?);
|
||||
}
|
||||
let merged_cli_overrides = cli_overrides
|
||||
.iter()
|
||||
.cloned()
|
||||
.chain(
|
||||
request_overrides
|
||||
.unwrap_or_default()
|
||||
.into_iter()
|
||||
.map(|(key, value)| (key, json_to_toml(value))),
|
||||
)
|
||||
|
||||
@@ -610,6 +610,7 @@ mod thread_processor_behavior_tests {
|
||||
Some(HashMap::from([
|
||||
("model_provider".to_string(), json!("request")),
|
||||
("features.plugins".to_string(), json!(true)),
|
||||
("bypass_hook_trust".to_string(), json!(true)),
|
||||
(
|
||||
"model_providers.session".to_string(),
|
||||
json!({
|
||||
@@ -626,6 +627,7 @@ mod thread_processor_behavior_tests {
|
||||
assert_eq!(config.model_provider_id, "session");
|
||||
assert_eq!(config.model_provider, session_provider);
|
||||
assert!(!config.features.enabled(Feature::Plugins));
|
||||
assert!(config.bypass_hook_trust);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -1259,6 +1259,9 @@ fn config_request_overrides_from_config(
|
||||
"web_search",
|
||||
Some(config.web_search_mode.value().to_string()),
|
||||
);
|
||||
if config.bypass_hook_trust {
|
||||
overrides.insert("bypass_hook_trust".to_string(), true.into());
|
||||
}
|
||||
Some(overrides)
|
||||
}
|
||||
|
||||
@@ -2131,7 +2134,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_lifecycle_params_forward_model_reasoning_and_service_tier() {
|
||||
async fn thread_lifecycle_params_forward_config_overrides_and_service_tier() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
let mut config = build_config(&temp_dir).await;
|
||||
config.model_reasoning_effort = Some(ReasoningEffort::High);
|
||||
@@ -2142,6 +2145,7 @@ mod tests {
|
||||
.web_search_mode
|
||||
.set(WebSearchMode::Disabled)
|
||||
.expect("test web search mode should be allowed");
|
||||
config.bypass_hook_trust = true;
|
||||
config.service_tier = Some(ServiceTier::Fast.request_value().to_string());
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
@@ -2175,6 +2179,7 @@ mod tests {
|
||||
("model_verbosity".to_string(), string("low")),
|
||||
("personality".to_string(), string("pragmatic")),
|
||||
("web_search".to_string(), string("disabled")),
|
||||
("bypass_hook_trust".to_string(), true.into()),
|
||||
]);
|
||||
assert_eq!(start.config, Some(expected_config.clone()));
|
||||
assert_eq!(resume.config, Some(expected_config.clone()));
|
||||
|
||||
+19
-5
@@ -1706,11 +1706,25 @@ async fn run_ratatui_app(
|
||||
},
|
||||
};
|
||||
|
||||
let startup_hooks_browser =
|
||||
match maybe_run_startup_hooks_review(&mut app_server, &mut tui, &config).await? {
|
||||
StartupHooksReviewOutcome::Continue => None,
|
||||
StartupHooksReviewOutcome::OpenHooksBrowser(data) => Some(data),
|
||||
};
|
||||
// Persistent app-server resumes may attach to an already-running thread,
|
||||
// where resume config overrides are ignored.
|
||||
let is_persistent_resume = !matches!(&app_server_target, AppServerTarget::Embedded)
|
||||
&& matches!(
|
||||
&session_selection,
|
||||
resume_picker::SessionSelection::Resume(_)
|
||||
);
|
||||
let bypass_hook_trust_for_startup_review = config.bypass_hook_trust && !is_persistent_resume;
|
||||
let startup_hooks_browser = match maybe_run_startup_hooks_review(
|
||||
&mut app_server,
|
||||
&mut tui,
|
||||
&config,
|
||||
bypass_hook_trust_for_startup_review,
|
||||
)
|
||||
.await?
|
||||
{
|
||||
StartupHooksReviewOutcome::Continue => None,
|
||||
StartupHooksReviewOutcome::OpenHooksBrowser(data) => Some(data),
|
||||
};
|
||||
|
||||
let app_result = App::run(
|
||||
&mut tui,
|
||||
|
||||
@@ -46,6 +46,7 @@ pub(crate) async fn maybe_run_startup_hooks_review(
|
||||
app_server: &mut AppServerSession,
|
||||
tui: &mut Tui,
|
||||
config: &Config,
|
||||
bypass_hook_trust: bool,
|
||||
) -> Result<StartupHooksReviewOutcome> {
|
||||
let cwd = config.cwd.to_path_buf();
|
||||
let response = match fetch_hooks_list(app_server.request_handle(), cwd.clone()).await {
|
||||
@@ -56,7 +57,7 @@ pub(crate) async fn maybe_run_startup_hooks_review(
|
||||
}
|
||||
};
|
||||
let entry = hooks_list_entry_for_cwd(response, &cwd);
|
||||
if review_needed_count(&entry) == 0 {
|
||||
if !review_is_needed(bypass_hook_trust, &entry) {
|
||||
return Ok(StartupHooksReviewOutcome::Continue);
|
||||
}
|
||||
|
||||
@@ -223,6 +224,10 @@ fn review_needed_count(entry: &HooksListEntry) -> usize {
|
||||
.count()
|
||||
}
|
||||
|
||||
fn review_is_needed(bypass_hook_trust: bool, entry: &HooksListEntry) -> bool {
|
||||
!bypass_hook_trust && review_needed_count(entry) > 0
|
||||
}
|
||||
|
||||
fn selection_item(name: &str, is_disabled: bool) -> SelectionItem {
|
||||
SelectionItem {
|
||||
name: name.to_string(),
|
||||
@@ -259,6 +264,7 @@ impl WidgetRef for &StandaloneSelectionView<'_> {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::review_is_needed;
|
||||
use super::selection_view;
|
||||
use crate::app_event::AppEvent;
|
||||
use crate::app_event_sender::AppEventSender;
|
||||
@@ -333,6 +339,16 @@ mod tests {
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bypass_hook_trust_suppresses_startup_review() {
|
||||
assert!(!review_is_needed(/*bypass_hook_trust*/ true, &entry()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn untrusted_hooks_need_review_without_bypass() {
|
||||
assert!(review_is_needed(/*bypass_hook_trust*/ false, &entry()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renders_prompt() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
|
||||
Reference in New Issue
Block a user