Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 90 additions & 0 deletions packages/core/src/handlers/approve.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* Handler for approving a pull request.
*
* Authenticates, submits an APPROVE review on the PR, and returns the
* review metadata. Optionally includes a review comment body.
*/

import { AuthError, Result, ValidationError } from "@outfitter/contracts";

import { detectAuth } from "../auth";
import type { HandlerContext } from "./types";

// =============================================================================
// Types
// =============================================================================

/** Input parameters for the approve handler. */
export interface ApproveInput {
/** PR number to approve */
pr: number;
/** Repository (owner/repo) */
repo: string;
/** Optional review comment body */
body?: string | undefined;
}

/** Structured output from the approve handler. */
export interface ApproveOutput {
/** Review node ID */
id: string;
/** URL to the review */
url?: string | undefined;
}

// =============================================================================
// Handler
// =============================================================================

/**
* Approve a pull request.
*
* Submits an APPROVE review via the GitHub REST API.
*
* @param input - PR number, repo, optional body
* @param ctx - Handler context with config, db, logger
* @returns Result with review ID and URL on success
*/
export async function approveHandler(
input: ApproveInput,
ctx: HandlerContext
): Promise<Result<ApproveOutput, Error>> {
const authResult = await detectAuth(ctx.config.github_token);
if (authResult.isErr()) {
return Result.err(
new AuthError({ message: authResult.error.message })
);
}

const { GitHubClient } = await import("../github");
const client = new GitHubClient(authResult.value.token);

const parts = input.repo.split("/");
if (parts.length !== 2 || !parts[0] || !parts[1]) {
return Result.err(
new ValidationError({ message: `Invalid repo format: ${input.repo}` })
);
}
const [owner, repo] = parts;
const reviewResult = await client.addReview(
owner,
repo,
input.pr,
"approve",
input.body
);

if (reviewResult.isErr()) {
return Result.err(reviewResult.error);
}

const review = reviewResult.value;
if (!review) {
return Result.ok({ id: "" });
}

return Result.ok({
id: review.id,
...(review.url && { url: review.url }),
});
}
234 changes: 234 additions & 0 deletions packages/core/src/handlers/edit.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
/**
* Handler for editing a pull request or comment.
*
* Polymorphic: edits PR metadata (title, body, base, labels, milestone)
* when given a PR number, or edits a comment body when given a comment ID.
*/

import {
AuthError,
NotFoundError,
Result,
ValidationError,
} from "@outfitter/contracts";

import { detectAuth } from "../auth";
import type { GitHubClient } from "../github";
import { getEntry } from "../repository";
import { classifyId, normalizeShortId, resolveShortId } from "../short-id";
import type { HandlerContext } from "./types";

// =============================================================================
// Types
// =============================================================================

/** Input parameters for the edit handler. */
export interface EditInput {
/** Target ID — PR number, short ID (@abc), or comment ID */
id: string;
/** Repository (owner/repo) */
repo: string;
/** New PR title (PR edit only) */
title?: string | undefined;
/** New PR/comment body */
body?: string | undefined;
/** New base branch (PR edit only) */
base?: string | undefined;
/** Labels to add (PR edit only) */
addLabels?: string[] | undefined;
/** Labels to remove (PR edit only) */
removeLabels?: string[] | undefined;
/** Milestone name (PR edit only) */
milestone?: string | undefined;
}

/** Structured output from the edit handler. */
export interface EditOutput {
/** What was edited */
type: "pr" | "comment";
/** Whether the edit succeeded */
ok: boolean;
}

// =============================================================================
// Handler
// =============================================================================

/**
* Edit a pull request or comment.
*
* When the ID resolves to a PR number, edits PR metadata via the REST API.
* When the ID resolves to a comment, edits the comment body.
*
* @param input - Target ID, repo, and fields to update
* @param ctx - Handler context with config, db, logger
* @returns Result indicating what was edited
*/
export async function editHandler(
input: EditInput,
ctx: HandlerContext
): Promise<Result<EditOutput, Error>> {
const authResult = await detectAuth(ctx.config.github_token);
if (authResult.isErr()) {
return Result.err(
new AuthError({ message: authResult.error.message })
);
}

const { GitHubClient } = await import("../github");
const client = new GitHubClient(authResult.value.token);

const parts = input.repo.split("/");
if (parts.length !== 2 || !parts[0] || !parts[1]) {
return Result.err(
new ValidationError({ message: `Invalid repo format: ${input.repo}` })
);
}
const [owner, repo] = parts;

// Determine if this is a PR number or a comment ID
const idType = classifyId(input.id);

if (idType === "pr_number") {
return editPr(client, owner, repo, Number(input.id), input);
}

// Resolve short ID to comment ID
let commentId = input.id;
if (idType === "short_id") {
const normalized = normalizeShortId(input.id);
const resolved = resolveShortId(normalized);
Comment on lines +99 to +100

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Populate short-ID cache before resolving edit target

This path resolves @shortid via resolveShortId only, but it never loads entries or calls buildShortIdCache, so in a fresh process the map is empty and valid short IDs fail with Could not resolve short ID. That makes edit by short ID non-functional unless some previous command happened to warm the in-memory cache first.

Useful? React with 👍 / 👎.

if (!resolved) {
return Result.err(
new NotFoundError({
message: `Could not resolve short ID: ${input.id}`,
resourceType: "entry",
resourceId: input.id,
})
);
}
commentId = resolved.fullId;
}

return editComment(client, owner, repo, commentId, input, ctx);
}

async function editPr(
client: GitHubClient,
owner: string,
repo: string,
prNumber: number,
input: EditInput
): Promise<Result<EditOutput, Error>> {
const updates: { title?: string; body?: string; base?: string } = {};
if (input.title) {
updates.title = input.title;
}
if (input.body) {
updates.body = input.body;
}
if (input.base) {
updates.base = input.base;
}

if (Object.keys(updates).length === 0 && !input.addLabels && !input.removeLabels && !input.milestone) {
return Result.err(
new ValidationError({ message: "No fields to update" })
);
}

if (Object.keys(updates).length > 0) {
const editResult = await client.editPullRequest(owner, repo, prNumber, updates);
if (editResult.isErr()) {
return Result.err(editResult.error);
}
}

// Handle labels
if (input.addLabels?.length) {
const addResult = await client.addLabels(owner, repo, prNumber, input.addLabels);
if (addResult.isErr()) {
return Result.err(addResult.error);
}
}
if (input.removeLabels?.length) {
const rmResult = await client.removeLabels(owner, repo, prNumber, input.removeLabels);
if (rmResult.isErr()) {
return Result.err(rmResult.error);
}
}

// Handle milestone
if (input.milestone) {
const msResult = await client.setMilestone(owner, repo, prNumber, input.milestone);
if (msResult.isErr()) {
return Result.err(msResult.error);
}
}

return Result.ok({ type: "pr", ok: true });
}

async function editComment(
client: GitHubClient,
owner: string,
repo: string,
commentId: string,
input: EditInput,
ctx: HandlerContext
): Promise<Result<EditOutput, Error>> {
if (!input.body) {
return Result.err(
new ValidationError({ message: "Comment edit requires a body" })
);
}

// Look up the entry to find the numeric comment ID for the REST API
const entry = getEntry(ctx.db, commentId, `${owner}/${repo}`);
if (!entry) {
return Result.err(
new NotFoundError({
message: `Entry not found: ${commentId}`,
resourceType: "entry",
resourceId: commentId,
})
);
}

// Issue comments use numeric IDs from the REST API
// The entry.id is the GraphQL node ID; we need the REST numeric ID
// For now, use the GraphQL mutation approach via editIssueComment
// which accepts the REST numeric comment ID
const numericId = extractNumericId(entry.id);
if (!numericId) {
return Result.err(
new ValidationError({
message: `Cannot determine numeric ID for comment: ${entry.id}`,
})
);
}
Comment on lines +202 to +209

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

extractNumericId(entry.id) won't work reliably - entry.database_id is optional and not always populated. Should use the same approach as apps/cli/src/commands/edit.ts:168-175 which tries entry.database_id first, then falls back to base64 decoding:

Suggested change
const numericId = extractNumericId(entry.id);
if (!numericId) {
return Result.err(
new ValidationError({
message: `Cannot determine numeric ID for comment: ${entry.id}`,
})
);
}
// Prefer stored database_id (synced from GitHub API)
const numericId = entry.database_id ?? extractNumericId(entry.id);
if (!numericId) {
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/handlers/edit.ts
Line: 202-209

Comment:
`extractNumericId(entry.id)` won't work reliably - `entry.database_id` is optional and not always populated. Should use the same approach as `apps/cli/src/commands/edit.ts:168-175` which tries `entry.database_id` first, then falls back to base64 decoding:

```suggestion
  // Prefer stored database_id (synced from GitHub API)
  const numericId = entry.database_id ?? extractNumericId(entry.id);
  if (!numericId) {
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex


const editResult = await client.editIssueComment(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Route review-comment edits to the review endpoint

The handler always calls editIssueComment, but GitHub review comments require the pull-review endpoint (editReviewComment). When the cached entry is a review_comment, this uses the wrong API route and edit requests fail for code-review comments, which are a primary feedback type in this tool.

Useful? React with 👍 / 👎.

owner,
repo,
numericId,
Comment on lines +211 to +214

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only handles issue comments - missing review_comment support. Check entry.subtype and call client.editReviewComment() for review comments (see apps/cli/src/commands/edit.ts:622-626)

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/handlers/edit.ts
Line: 211-214

Comment:
Only handles issue comments - missing `review_comment` support. Check `entry.subtype` and call `client.editReviewComment()` for review comments (see `apps/cli/src/commands/edit.ts:622-626`)

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

input.body
);
if (editResult.isErr()) {
return Result.err(editResult.error);
}

return Result.ok({ type: "comment", ok: true });
}

/** Extract numeric ID from various GitHub ID formats. */
function extractNumericId(id: string): number | null {
// Try direct numeric
const num = Number(id);
if (!Number.isNaN(num) && num > 0) {
return num;
}
// Try extracting trailing number from node ID patterns
Comment on lines +222 to +231

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

extractNumericId() is too simplistic - GitHub node IDs are base64-encoded (e.g., "IC_kwDO..."). The regex won't extract the numeric ID correctly. Use the base64 decoding approach from apps/cli/src/commands/edit.ts:148-162:

Suggested change
}
/** Extract numeric ID from various GitHub ID formats. */
function extractNumericId(id: string): number | null {
// Try direct numeric
const num = Number(id);
if (!Number.isNaN(num) && num > 0) {
return num;
}
// Try extracting trailing number from node ID patterns
function extractNumericId(id: string): number | null {
// Try direct numeric
const num = Number(id);
if (!Number.isNaN(num) && num > 0) {
return num;
}
// Try base64 decoding for GraphQL node IDs
try {
const decoded = Buffer.from(id, "base64").toString("utf8");
const matches = decoded.match(/\d+/g);
const last = matches?.at(-1);
return last ? Number(last) : null;
} catch {
return null;
}
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/handlers/edit.ts
Line: 222-231

Comment:
`extractNumericId()` is too simplistic - GitHub node IDs are base64-encoded (e.g., "IC_kwDO..."). The regex won't extract the numeric ID correctly. Use the base64 decoding approach from `apps/cli/src/commands/edit.ts:148-162`:

```suggestion
function extractNumericId(id: string): number | null {
  // Try direct numeric
  const num = Number(id);
  if (!Number.isNaN(num) && num > 0) {
    return num;
  }
  // Try base64 decoding for GraphQL node IDs
  try {
    const decoded = Buffer.from(id, "base64").toString("utf8");
    const matches = decoded.match(/\d+/g);
    const last = matches?.at(-1);
    return last ? Number(last) : null;
  } catch {
    return null;
  }
}
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

const match = id.match(/(\d+)$/);
return match ? Number(match[1]) : null;
Comment on lines +231 to +233

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Stop deriving REST comment IDs from node-id suffixes

Extracting the REST comment ID by regexing trailing digits from entry.id is unreliable because entry.id is an opaque GraphQL node ID, not the REST numeric ID. This causes frequent false failures (Cannot determine numeric ID) and can also produce an unrelated number when a node ID happens to end with digits, potentially editing the wrong comment.

Useful? React with 👍 / 👎.

}
15 changes: 15 additions & 0 deletions packages/core/src/handlers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ export {
type AckItemResult,
type AckOutput,
} from "./ack";
export {
approveHandler,
type ApproveInput,
type ApproveOutput,
} from "./approve";
export {
closeHandler,
type CloseInput,
Expand All @@ -20,11 +25,21 @@ export {
type DoctorInput,
type DoctorOutput,
} from "./doctor";
export {
editHandler,
type EditInput,
type EditOutput,
} from "./edit";
export {
queryHandler,
type QueryInput,
type QueryOutput,
} from "./query";
export {
rejectHandler,
type RejectInput,
type RejectOutput,
} from "./reject";
export {
replyHandler,
type ReplyInput,
Expand Down
Loading