Skip to content
Merged
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
46 changes: 37 additions & 9 deletions src/lib/v5/mcp-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
getTask,
getWishGroups,
listTasks,
listWishSlugs,
} from './task-state.js';

// ============================================================================
Expand Down Expand Up @@ -97,17 +98,42 @@ function currentBranch(cwd: string): string | null {
}

/**
* Parse a `wish/<slug>-<group>` branch into `{ wish, group }`. The group is the
* final `-`-delimited segment; the slug is everything before it (slugs may
* themselves contain hyphens, e.g. `wish/genie-mcp-g2` → `genie-mcp` / `g2`).
* Returns `null` when the branch is not a wish worktree branch.
* Resolve a `wish/<slug>[-<group>]` branch into `{ wish, group }`. Both slug and
* group may contain hyphens, so a raw last-dash split is ambiguous
* (`wish/genie-mcp` is the `genie-mcp` wish with no group, NOT a `genie` wish
* with an `mcp` group). Disambiguate against the db, most-authoritative first:
* 1. a `<slug>-<group>` where BOTH the slug is known AND `<group>` is a real
* group of it → a launch worktree (beats a same-named top-level slug);
* 2. exact known slug → top-level branch, group = null;
* 3. longest known slug that is a prefix + `-<group>` (group unverified);
* 4. no known wish (brand-new branch) → last-dash heuristic, else whole rest.
* Returns `null` only when the branch is not a `wish/…` branch.
*/
function parseWishBranch(branch: string): { wish: string; group: string } | null {
function resolveWishBranch(db: Database | null, branch: string): { wish: string; group: string | null } | null {
const rest = branch.startsWith('wish/') ? branch.slice('wish/'.length) : null;
if (!rest) return null;
const known = db ? listWishSlugs(db) : []; // longest-first
// 1. Verified launch worktree: the group actually exists on the prefix wish.
if (db) {
for (const slug of known) {
if (!rest.startsWith(`${slug}-`)) continue;
const group = rest.slice(slug.length + 1);
if (group && getWishGroups(db, slug).some((g) => g.name === group)) return { wish: slug, group };
}
}
// 2. Exact known slug → top-level branch (no group).
if (known.includes(rest)) return { wish: rest, group: null };

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 Check group-branch candidates before exact wish matches

When a launched group branch's <slug>-<group> string is also a known wish slug, this early exact match resolves the branch as a top-level wish instead of the group worktree. genie launch creates branches as wish/${slug}-${name} and both slugs and groups allow hyphens, so with wish foo-bar group baz plus a separate wish foo-bar-baz, the foo-bar/baz worktree now receives all tasks for foo-bar-baz rather than its own group context. Please validate known wish/group (or task group) candidates before treating the entire suffix as a top-level wish.

Useful? React with 👍 / 👎.

// 3. Longest known slug that is a prefix (group unverified) → best guess.
for (const slug of known) {
if (rest.startsWith(`${slug}-`)) {
const group = rest.slice(slug.length + 1);
if (group) return { wish: slug, group };
}
}
// 4. No known wish yet → last-dash heuristic, else the whole rest as the wish.
const dash = rest.lastIndexOf('-');
if (dash <= 0 || dash === rest.length - 1) return null;
return { wish: rest.slice(0, dash), group: rest.slice(dash + 1) };
if (dash > 0 && dash < rest.length - 1) return { wish: rest.slice(0, dash), group: rest.slice(dash + 1) };
return { wish: rest, group: null };
}

// ============================================================================
Expand Down Expand Up @@ -190,10 +216,12 @@ interface WorktreeContextPayload {

function genieWorktreeContext(ctx: ToolContext, args: Record<string, unknown>): WorktreeContextPayload {
const branch = argString(args, 'branch') ?? currentBranch(ctx.cwd);
const parsed = branch ? parseWishBranch(branch) : null;
const parsed = branch ? resolveWishBranch(ctx.db, branch) : null;

if (parsed) {
const tasks = ctx.db ? listTasks(ctx.db, { wish: parsed.wish }).filter((t) => t.group === parsed.group) : [];
const wishTasks = ctx.db ? listTasks(ctx.db, { wish: parsed.wish }) : [];
// Top-level wish branch (no group) → all of the wish's tasks; a group branch → just that group.
const tasks = parsed.group === null ? wishTasks : wishTasks.filter((t) => t.group === parsed.group);
return { branch, resolved: true, wish: parsed.wish, group: parsed.group, tasks: tasks.map(toSummary) };
}

Expand Down
18 changes: 18 additions & 0 deletions src/lib/v5/task-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,24 @@ export function getWishGroups(db: Database, wish: string): WishGroupRow[] {
return rows.map(mapWishGroup);
}

/**
* Distinct known wish slugs (from tasks + wish_groups), longest first. Used to
* disambiguate a `wish/<slug>-<group>` branch when the slug itself contains
* hyphens (`genie-mcp` vs a `genie` wish with an `mcp` group).
*/
export function listWishSlugs(db: Database): string[] {
const rows = db
.query(
// UNION already de-duplicates; order longest-first for prefix disambiguation.
`SELECT wish FROM (
SELECT wish FROM tasks WHERE wish IS NOT NULL
UNION SELECT wish FROM wish_groups WHERE wish IS NOT NULL
) ORDER BY LENGTH(wish) DESC`,
)
.all() as Array<{ wish: string }>;
return rows.map((r) => r.wish);
}
Comment on lines +619 to +630

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The GROUP BY wish clause is redundant here because the UNION operator already implicitly removes duplicate rows. Removing it simplifies the query and avoids unnecessary grouping overhead.

Additionally, since this is a read-only MCP tool designed to degrade gracefully and never throw, wrapping the query in a try-catch block to return [] on error is highly recommended. This prevents potential unhandled exceptions (e.g., if the database is in an intermediate/uninitialized state where the tasks or wish_groups tables do not exist yet) from crashing the server.

Suggested change
export function listWishSlugs(db: Database): string[] {
const rows = db
.query(
`SELECT wish FROM (
SELECT wish FROM tasks WHERE wish IS NOT NULL
UNION SELECT wish FROM wish_groups WHERE wish IS NOT NULL
) GROUP BY wish ORDER BY LENGTH(wish) DESC`,
)
.all() as Array<{ wish: string }>;
return rows.map((r) => r.wish);
}
export function listWishSlugs(db: Database): string[] {
try {
const rows = db
.query(
"SELECT wish FROM (SELECT wish FROM tasks WHERE wish IS NOT NULL UNION SELECT wish FROM wish_groups WHERE wish IS NOT NULL) ORDER BY LENGTH(wish) DESC",
)
.all() as Array<{ wish: string }>;
return rows.map((r) => r.wish);
} catch {
return [];
}
}


function getWishGroup(db: Database, wish: string, name: string): WishGroupRow | null {
const row = db.query('SELECT * FROM wish_groups WHERE wish = ? AND name = ?').get(wish, name) as
| Parameters<typeof mapWishGroup>[0]
Expand Down
105 changes: 105 additions & 0 deletions src/term-commands/mcp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,111 @@ describe('mcp absent-db degrade', () => {
expect(payload.counts.total).toBe(0);
expect(payload.tasks).toEqual([]);
});

test('reopens a db created AFTER the server started (no stale empty board)', async () => {
// Server starts against a repo with no genie.db (null handle), THEN the db
// is created mid-session — a per-call reopen must pick it up, not serve empty.
const proc = Bun.spawn(['bun', GENIE, 'mcp'], {
cwd: repo,
stdin: 'pipe',
stdout: 'pipe',
stderr: 'pipe',
env: { ...process.env, NO_COLOR: '1', GENIE_TEST_SKIP_PGSERVE: '1' },
});
const reader = proc.stdout.getReader();
const decoder = new TextDecoder();
let out = '';
// Synchronize: wait for the initialize reply BEFORE seeding, which proves the
// server's startup read-only open already ran against an absent db (null) —
// otherwise the test could seed first and pass without exercising the reopen.
proc.stdin.write(`${JSON.stringify(INIT)}\n`);
while (!out.includes('"serverInfo"')) {
const { value, done } = await reader.read();
if (done) break;
out += decoder.decode(value, { stream: true });
}
seed(repo); // create .genie/genie.db + tasks AFTER the startup open saw nothing
proc.stdin.write(
`${JSON.stringify({ jsonrpc: '2.0', id: 20, method: 'tools/call', params: { name: 'genie_board', arguments: {} } })}\n`,
);
await proc.stdin.end();
while (true) {
const { value, done } = await reader.read();
if (done) break;
out += decoder.decode(value, { stream: true });
}
await proc.exited;
const responses = out
.split('\n')
.filter((l) => l.trim().length > 0)
.map((l) => JSON.parse(l) as RpcResponse);
const res = responses.find((r) => r.id === 20);
const payload = toolPayload<{ counts: { total: number } }>(res!);
expect(payload.counts.total).toBeGreaterThan(0); // saw the db created mid-session
});
});

describe('mcp worktree branch resolution', () => {
test('disambiguates a hyphenated wish slug against known wishes', async () => {
seed(repo); // wish 'genie-mcp' (slug has a hyphen), groups g1/g2, a task in g2
// Top-level `wish/genie-mcp` must resolve to the genie-mcp wish (group null),
// NOT a mis-split `genie` wish with an `mcp` group.
const top = await driveMcp(repo, [
INIT,
INITIALIZED,
{
jsonrpc: '2.0',
id: 30,
method: 'tools/call',
params: { name: 'genie_worktree_context', arguments: { branch: 'wish/genie-mcp' } },
},
]);
const topP = toolPayload<{ resolved: boolean; wish: string; group: string | null }>(top.find((r) => r.id === 30)!);
expect(topP.resolved).toBe(true);
expect(topP.wish).toBe('genie-mcp');
expect(topP.group).toBeNull();

// A group branch `wish/genie-mcp-g2` → genie-mcp / g2 (last-dash is correct here).
const grp = await driveMcp(repo, [
INIT,
INITIALIZED,
{
jsonrpc: '2.0',
id: 31,
method: 'tools/call',
params: { name: 'genie_worktree_context', arguments: { branch: 'wish/genie-mcp-g2' } },
},
]);
const grpP = toolPayload<{ wish: string; group: string | null }>(grp.find((r) => r.id === 31)!);
expect(grpP.wish).toBe('genie-mcp');
expect(grpP.group).toBe('g2');
});

test('a launch group branch beats a same-named top-level wish slug', async () => {
// Ambiguous collision: a `genie` wish WITH a real `mcp` group, AND a separate
// `genie-mcp` wish. `wish/genie-mcp` must resolve to the verified launch
// worktree (genie / mcp), not the exact-slug top-level `genie-mcp` wish.
const db = openDb({ cwd: repo });
const board = createBoard(db, 'repo');
createTask(db, { title: 'a', boardId: board.id, wish: 'genie', group: 'mcp' });
createWishGroups(db, 'genie', [{ name: 'mcp' }]);
createTask(db, { title: 'b', boardId: board.id, wish: 'genie-mcp', group: 'g1' });
createWishGroups(db, 'genie-mcp', [{ name: 'g1' }]);
db.close();
const res = await driveMcp(repo, [
INIT,
INITIALIZED,
{
jsonrpc: '2.0',
id: 32,
method: 'tools/call',
params: { name: 'genie_worktree_context', arguments: { branch: 'wish/genie-mcp' } },
},
]);
const p = toolPayload<{ wish: string; group: string | null }>(res.find((r) => r.id === 32)!);
expect(p.wish).toBe('genie');
expect(p.group).toBe('mcp');
});
});

// ============================================================================
Expand Down
12 changes: 9 additions & 3 deletions src/term-commands/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ export async function runMcpServer(): Promise<void> {
// Lazy: the read-only bun:sqlite open + tools load here, not at genie startup.
const { MCP_TOOLS, openReadonlyDb } = await import('../lib/v5/mcp-tools.js');
const cwd = process.cwd();
const db = openReadonlyDb(cwd);
const ctx: ToolContext = { db, cwd };
// Single source of truth for the handle: the per-call reopen below writes back
// to ctx.db, and close() reads ctx.db — so a mid-session reopen is always the
// one that gets closed (no stale/leaked handle).
const ctx: ToolContext = { db: openReadonlyDb(cwd), cwd };

const toolByName = new Map(MCP_TOOLS.map((t) => [t.name, t] as const));

Expand All @@ -91,6 +93,10 @@ export async function runMcpServer(): Promise<void> {
});
return;
}
// The db may have been absent when the server started (null handle → empty
// board). Re-attempt the read-only open per call so a db created mid-session
// (e.g. a fresh `genie init`) is picked up instead of serving empty forever.
if (!ctx.db) ctx.db = openReadonlyDb(cwd);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

When ctx.db is dynamically opened mid-session here, the local db variable (which was initialized to null at startup) is not updated. Consequently, the rl.on('close', ...) handler at the end of runMcpServer (which calls db?.close()) will not close this newly opened database connection, resulting in a resource/file descriptor leak.

To fix this, please update the close handler to close ctx.db instead of db:

rl.on('close', () => {
  ctx.db?.close();
  ...
});

const payload = tool.handler(ctx, args);
ok(id, {
content: [{ type: 'text', text: JSON.stringify(payload) }],
Expand Down Expand Up @@ -162,7 +168,7 @@ export async function runMcpServer(): Promise<void> {

await new Promise<void>((resolve) => {
rl.on('close', () => {
db?.close();
ctx.db?.close();
// Flush stdout BEFORE exiting: the empty-write callback fires after the
// stream's buffer (including all prior response writes) drains to the OS,
// so the final response is never truncated. See SPIKE.md flush note.
Expand Down