Skip to content

Conversation

@kangju2000
Copy link
Member

  • Add cancel command implementation to mark active intents as cancelled
    • Add cancel command to CLI interface
    • Follow same pattern as existing finish

@kangju2000 kangju2000 requested a review from jcha0713 August 2, 2025 06:57
@kangju2000 kangju2000 self-assigned this Aug 2, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @kangju2000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a new cancel command to the CLI, enabling users to manage their active intents by explicitly marking them as cancelled. This enhancement provides a crucial capability for controlling intent lifecycles, following the established pattern of existing commands like finish.

Highlights

  • New CLI Command: I've added a new cancel command to the command-line interface. This command allows users to explicitly mark their active intents as cancelled, providing a direct way to manage ongoing processes.
  • Core Logic for Intent Cancellation: I've implemented the core logic for intent cancellation in a new cancel function. This function identifies the active intent associated with a given branch and updates its status to 'cancelled' within the database.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a cancel command to the CLI, allowing users to mark the currently active intent as 'cancelled'. The implementation adds a new command to the commander interface and a corresponding core logic function to update the database.

The changes are well-structured and follow the existing patterns in the codebase. I've identified a couple of areas for improvement in the new cancel.ts file:

  • The error handling in the cancel command is redundant and can lead to duplicated error messages. I've suggested simplifying it to let the CLI layer handle error presentation.
  • The use of a hardcoded string for the 'cancelled' status could be improved by using a shared constant to enhance maintainability.

Overall, this is a solid addition. Addressing these points will make the code more robust and maintainable.

Comment on lines +6 to +32
export async function cancel({
branchId,
}: {
branchId: string;
}): Promise<Intent> {
try {
const activeIntent = await findActiveIntent({ branchId });

if (!activeIntent) {
throw new Error("No active intent found for this branch");
}

return db
.update(intents)
.set({
status: "cancelled",
})
.where(eq(intents.id, activeIntent.id))
.returning()
.get();
} catch (error) {
console.error("Failed to cancel intent: ", error);
throw new Error(
`Failed to cancel intent: ${error instanceof Error ? error.message : "Unknown error"}`,
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The try...catch block in this function leads to redundant error messages and includes side effects (logging) that are better handled by the caller.

  1. Redundant Error Messages: The catch block throws a new error with the message Failed to cancel intent: .... The caller in src/cli.ts also catches this error and prepends its own Failed to cancel intent: message, resulting in a duplicated and confusing output for the user (e.g., Failed to cancel intent: Failed to cancel intent: No active intent found...).
  2. Side Effects: The console.error call within this core function is a side effect that makes it less reusable and testable. It's better practice for library-like functions to be pure and not interact with the console. The CLI layer is the appropriate place to handle user-facing output.

By removing the try...catch block, you simplify the code, avoid the duplicated error message, and improve the separation of concerns. Errors from findActiveIntent or db.update will naturally propagate to the CLI handler, which is already set up to manage them correctly.

export async function cancel({
  branchId,
}: {
  branchId: string;
}): Promise<Intent> {
  const activeIntent = await findActiveIntent({ branchId });

  if (!activeIntent) {
    throw new Error("No active intent found for this branch");
  }

  return db
    .update(intents)
    .set({
      status: "cancelled",
    })
    .where(eq(intents.id, activeIntent.id))
    .returning()
    .get();
}

return db
.update(intents)
.set({
status: "cancelled",
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The status "cancelled" is hardcoded here. The database schema (src/core/db/schema.ts) refers to an INTENT_STATUS constant for the status column, which suggests a centralized place for these values. Using a hardcoded string here can lead to inconsistencies and maintenance issues if the status values ever change.

To improve maintainability and type safety, it would be better to use a shared constant or enum for intent statuses throughout the application, instead of raw strings. This would apply here and in other places like findActiveIntent.ts.

Copy link
Collaborator

@jcha0713 jcha0713 left a comment

Choose a reason for hiding this comment

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

This one looks good as well. Let's get it merged.

@kangju2000 kangju2000 merged commit bba5bac into next Aug 2, 2025
3 checks passed
@kangju2000 kangju2000 deleted the feat/cancel-action branch August 2, 2025 11:30
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