Skip to content

feat(context): load and execute context hooks #1176

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.

Sign up for GitHub

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

Merged
hayemaxi merged 2 commits into from
Apr 13, 2025
Merged

feat(context): load and execute context hooks #1176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b410760
feat(context): load and execute context hooks
hayemaxi Apr 10, 2025
edaf69d
update enable hook func names, add serde default
hayemaxi Apr 13, 2025
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
147 changes: 142 additions & 5 deletions crates/q_cli/src/cli/chat/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::io::Write;
use std::path::{
Path,
PathBuf,
Expand All @@ -17,13 +18,21 @@ use serde::{
Serialize,
};

use super::hooks::{
Hook,
HookExecutor,
};
use crate::cli::chat::hooks::HookConfig;

pub const AMAZONQ_FILENAME: &str = "AmazonQ.md";

/// Configuration for context files, containing paths to include in the context.
#[derive(Debug, Clone, Serialize, Deserialize, Default)]
#[serde(default)]
pub struct ContextConfig {
/// List of file paths or glob patterns to include in the context.
pub paths: Vec<String>,
pub hooks: HookConfig,
}

#[allow(dead_code)]
Expand All @@ -40,6 +49,8 @@ pub struct ContextManager {

/// Context configuration for the current profile.
pub profile_config: ContextConfig,

pub hook_executor: HookExecutor,
}

#[allow(dead_code)]
Expand Down Expand Up @@ -67,6 +78,7 @@ impl ContextManager {
global_config,
current_profile,
profile_config,
hook_executor: HookExecutor::new(),
})
}

Expand Down Expand Up @@ -157,11 +169,7 @@ impl ContextManager {
/// A Result indicating success or an error
pub async fn remove_paths(&mut self, paths: Vec<String>, global: bool) -> Result<()> {
// Get reference to the appropriate config
let config = if global {
&mut self.global_config
} else {
&mut self.profile_config
};
let config = self.get_config_mut(global);

// Track if any paths were removed
let mut removed_any = false;
Expand Down Expand Up @@ -433,6 +441,134 @@ impl ContextManager {
}
Ok(())
}

fn get_config_mut(&mut self, global: bool) -> &mut ContextConfig {
if global {
&mut self.global_config
} else {
&mut self.profile_config
}
}

/// Add hooks to the context config. If another hook with the same name already exists, throw an
/// error.
///
/// # Arguments
/// * `hook` - name of the hook to delete
/// * `global` - If true, the add to the global config. If false, add to the current profile
/// config.
/// * `conversation_start` - If true, add the hook to conversation_start. Otherwise, it will be
/// added to per_prompt.
pub async fn add_hook(&mut self, hook: Hook, global: bool, conversation_start: bool) -> Result<()> {
Copy link
Contributor

@brandonskiser brandonskiser Apr 11, 2025

Choose a reason for hiding this comment

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

Reason why we need to force namespacing concerns on the user rather than handle it ourselves? Like a hook id could be global/{name} / {profile_name}/{name} if multiple {name} hooks exist

Copy link
Contributor Author

@hayemaxi hayemaxi Apr 11, 2025

Choose a reason for hiding this comment

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

This is the same issue, what if the user tries to add 2 {name} hooks to global? Instead of throwing an errors, we can rename hooks with duplicate names as they arrive, e.g.

hook add ... "random_number" --global
hook add ... "random_number" --global # again
# ... list hooks
global:
- random_number
- random_number (1)

This also handles the issue of bypassing the duplicate name hook check by writing to the config manually.

Copy link
Contributor

@brandonskiser brandonskiser Apr 11, 2025

Choose a reason for hiding this comment

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

oh are you saying that you agree with the profile/hook_name pattern where we want to support hook_name across multiple profiles? e.g.

profile_1:
- hook_1
- hook_2

profile_2:
- hook_1

Then referencing hook_2 is fine, but we'll give an error stating that the user has profile_1/hook_1 and profile_2/hook_1

But, then you're asking about the case where we have multiple hook names tied to the same profile?

So we can either:

  • throw an error stating a hook with that name already exists
  • or, create a new hook and just call it hook_name_{rand_num}

Copy link
Contributor Author

@hayemaxi hayemaxi Apr 13, 2025

Choose a reason for hiding this comment

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

or, create a new hook and just call it hook_name_{rand_num}

This sounds good to me. Otherwise, I am hesitant to explicitly make a hook id with global/ or profile/ prefix since it is just additional complexity. Duplicate naming across profiles should be acceptable since they are stored in separate config files (and cannot be active at the same time).

Instead I was thinking we can just enforce it this way:

  • --global flag (like other context commands) controls for the global config, otherwise the profile config
  • names must be unique within a single config file, e.g. within global, or within a single profile.
    • if the user tries to add a non-unique name, add an increasing number on the end
    • if they add one via manually editing config files, fix this on startup by adding an increasing number to it
  • 2 execution caches, global and profile. We can clear the profile cache if they switch profiles

Copy link
Contributor Author

@hayemaxi hayemaxi Apr 13, 2025

Choose a reason for hiding this comment

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

Will follow up in a new PR.

if self.num_hooks_with_name(&hook.name) > 0 {
return Err(eyre!(
"Cannot add hook, another hook with this name already exists in global or profile context."
));
}

let config = self.get_config_mut(global);

let hook_vec = if conversation_start {
&mut config.hooks.conversation_start
} else {
&mut config.hooks.per_prompt
};

hook_vec.push(hook);
self.save_config(global).await
}

fn num_hooks_with_name(&self, name: &str) -> usize {
self.global_config
.hooks
.conversation_start
.iter()
.chain(self.global_config.hooks.per_prompt.iter())
.chain(self.profile_config.hooks.conversation_start.iter())
.chain(self.profile_config.hooks.per_prompt.iter())
.filter(|h| h.name == name)
.count()
}

/// Delete hook(s) by name
/// # Arguments
/// * `name` - name of the hook to delete
/// * `global` - If true, the delete from the global config. If false, delete from the current
/// profile config
pub async fn remove_hook(&mut self, name: &str, global: bool) -> Result<()> {
let config = self.get_config_mut(global);

config.hooks.conversation_start.retain(|h| h.name != name);
config.hooks.per_prompt.retain(|h| h.name != name);

self.save_config(global).await
}

/// Sets the "disabled" field on any [`Hook`] with the given name
/// # Arguments
/// * `disable` - Set "disabled" field to this value
pub async fn set_hook_disabled(&mut self, name: &str, global: bool, disable: bool) -> Result<()> {
let config = self.get_config_mut(global);

config
.hooks
.conversation_start
.iter_mut()
.chain(config.hooks.per_prompt.iter_mut())
.filter(|h| h.name == name)
.for_each(|h| h.disabled = disable);

self.save_config(global).await
}

/// Sets the "disabled" field on all [`Hook`]s
/// # Arguments
/// * `disable` - Set all "disabled" fields to this value
pub async fn set_all_hooks_disabled(&mut self, global: bool, disable: bool) -> Result<()> {
let config = self.get_config_mut(global);

config
.hooks
.conversation_start
.iter_mut()
.chain(config.hooks.per_prompt.iter_mut())
.for_each(|h| h.disabled = disable);

self.save_config(global).await
}

/// Run all the currently enabled hooks from both the global and profile contexts
/// # Arguments
/// * `updates` - output stream to write hook run status to
/// # Returns
/// A vector containing pairs of a [`Hook`] definition and its execution output
pub async fn run_hooks(&mut self, updates: &mut impl Write) -> Vec<(Hook, String)> {
let mut hooks: Vec<&Hook> = Vec::new();

// Collect all conversation start hooks
hooks.extend(
self.global_config
.hooks
.conversation_start
.iter_mut()
.chain(self.profile_config.hooks.conversation_start.iter_mut())
.map(|h| {
h.is_conversation_start = true;
&*h
}),
);

// Collect all per-prompt hooks
hooks.extend(
self.global_config
.hooks
.per_prompt
.iter()
.chain(self.profile_config.hooks.per_prompt.iter()),
);

self.hook_executor.run_hooks(hooks, updates).await
}
}

fn profile_dir_path(ctx: &Context, profile_name: &str) -> Result<PathBuf> {
Expand Down Expand Up @@ -464,6 +600,7 @@ async fn load_global_config(ctx: &Context) -> Result<ContextConfig> {
"README.md".to_string(),
AMAZONQ_FILENAME.to_string(),
],
hooks: HookConfig::default(),
})
}
}
Expand Down
Loading
Loading