Skip to content

Conversation

@hayemaxi
Copy link
Contributor

@hayemaxi hayemaxi commented Apr 10, 2025

RFC: #1050

This PR begins implements the required logic for executing hooks. This PR has pending follow ups to handle the remaining items. Note that the logic is currently inaccessible from a normal chat session.

It implements:

  • "Hooks", which are defined in the context json files. They are loaded along with the normal context.
    • "conversation_start" hooks that run once and are attached to the top of the context,
    • "per_prompt" hooks that run each time there is a prompt, and are attached to the user prompt.
  • Start with 1 hook type, "inline hooks", which simple execute a bash command.
  • A HookExecutor to asynchronously call hooks in order from the configs.
  • Logic to add/remove/enable/disable hooks via commands
  • A cache to hold recent executions of hooks to avoid calling them too much.
    • "conversation_start" hooks are held in cache indefinitely. "per_prompt" hooks can have
  • The bash execution logic from exceute_bash tool was moved to a helper function so that it can be re-used here.

Remaining items in another PR:

  • Call the hooks during a chat session and attach them to context appropriately
  • Commands to enable, disable, add, and show hooks
  • Add Criticality as according to the spec
  • Handle user having multiple hooks with the same name (enforce uniqueness)
  • Tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 2.36486% with 289 lines in your changes missing coverage. Please review.

Project coverage is 13.96%. Comparing base (bcd0c0b) to head (edaf69d).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
crates/q_cli/src/cli/chat/hooks.rs 3.96% 121 Missing ⚠️
crates/q_cli/src/cli/chat/context.rs 2.29% 85 Missing ⚠️
crates/q_cli/src/cli/chat/tools/execute_bash.rs 0.00% 83 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
+ Coverage   13.94%   13.96%   +0.01%     
==========================================
  Files        2363     2365       +2     
  Lines      204483   204847     +364     
  Branches   184847   185211     +364     
==========================================
+ Hits        28520    28604      +84     
- Misses     174562   174837     +275     
- Partials     1401     1406       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hayemaxi hayemaxi force-pushed the context-hooks-final branch 2 times, most recently from 38fb2b6 to c631c29 Compare April 10, 2025 19:01
@hayemaxi hayemaxi marked this pull request as ready for review April 10, 2025 19:20
@hayemaxi hayemaxi requested a review from a team April 10, 2025 19:20
RFC: aws#1050

This PR begins implements the required logic for executing hooks. This PR has pending follow ups to handle the remaining items.
Note that the logic is currently inaccessible from a normal chat session.

It implements:
- "Hooks", which are defined in the context json files. They are loaded along with the normal context.
  - "conversation_start" hooks that run once and are attached to the top of the context,
  - "per_prompt" hooks that run each time there is a prompt, and are attached to the user prompt.
- Start with 1 hook type, "inline hooks", which simple execute a bash command.
- A HookExecutor to asynchronously call hooks in order from the configs.
- Logic to add/remove/enable/disable hooks via commands
- A cache to hold recent executions of hooks to avoid calling them too much.
  - "conversation_start" hooks are held in cache indefinitely. "per_prompt" hooks can have
- The bash execution logic from `exceute_bash` tool was moved to a helper function so that it can be re-used here.

Remaining items in another PR:
- Call the hooks during a chat session and attach them to context appropriately
- Commands to enable, disable, add, and show hooks
- Add `Criticality` as according to the spec
- Handle user having multiple hooks with the same name (enforce uniqueness)
- Remaining tests
@hayemaxi hayemaxi force-pushed the context-hooks-final branch from c631c29 to b410760 Compare April 10, 2025 19:46
/// Unique name of the hook
pub name: String,

pub r#type: HookType,
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about r#

/// 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

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

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

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

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

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.

@hayemaxi hayemaxi force-pushed the context-hooks-final branch from c9e550b to edaf69d Compare April 13, 2025 19:25
@hayemaxi hayemaxi merged commit e42f218 into aws:main Apr 13, 2025
10 checks passed
hayemaxi added a commit to hayemaxi/amazon-q-developer-cli that referenced this pull request Apr 14, 2025
Follow up to: aws#1176
RFC: aws#1050

- Add new command `/context hooks`
  - subcommands to add/remove and enable/disable hooks
- Context hooks are called then added to the beginning of the chat history as context, or to each prompt, depending on what is chosen.
hayemaxi added a commit that referenced this pull request Apr 14, 2025
Follow up to: #1176
RFC: #1050

- Add new command `/context hooks`
  - subcommands to add/remove and enable/disable hooks
- Context hooks are called then added to the beginning of the chat history as context, or to each prompt, depending on what is chosen.
jsamuel1 pushed a commit that referenced this pull request Apr 15, 2025
* feat(context): load and execute context hooks

RFC: #1050

This PR begins implements the required logic for executing hooks. This PR has pending follow ups to handle the remaining items.
Note that the logic is currently inaccessible from a normal chat session.

It implements:
- "Hooks", which are defined in the context json files. They are loaded along with the normal context.
  - "conversation_start" hooks that run once and are attached to the top of the context,
  - "per_prompt" hooks that run each time there is a prompt, and are attached to the user prompt.
- Start with 1 hook type, "inline hooks", which simple execute a bash command.
- A HookExecutor to asynchronously call hooks in order from the configs.
- Logic to add/remove/enable/disable hooks via commands
- A cache to hold recent executions of hooks to avoid calling them too much.
  - "conversation_start" hooks are held in cache indefinitely. "per_prompt" hooks can have
- The bash execution logic from `exceute_bash` tool was moved to a helper function so that it can be re-used here.

Remaining items in another PR:
- Call the hooks during a chat session and attach them to context appropriately
- Commands to enable, disable, add, and show hooks
- Add `Criticality` as according to the spec
- Handle user having multiple hooks with the same name (enforce uniqueness)
- Remaining tests

* update enable hook func names, add serde default
jsamuel1 pushed a commit that referenced this pull request Apr 15, 2025
Follow up to: #1176
RFC: #1050

- Add new command `/context hooks`
  - subcommands to add/remove and enable/disable hooks
- Context hooks are called then added to the beginning of the chat history as context, or to each prompt, depending on what is chosen.
This was referenced Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants