Repository navigation
fix(mcp): db-backed branch resolution + reopen db created mid-session (2 PR-review P2s) - #2512
Conversation
Two P2s from the #2510/#2509 PR review (Codex): - Worktree branch parsing split on the last '-', so a top-level 'wish/genie-mcp' mis-resolved to a 'genie' wish with an 'mcp' group. Now disambiguated against the KNOWN wish slugs (new listWishSlugs, longest-first): exact slug → top-level (group null); else longest known prefix + '-<group>'; heuristic only when the wish isn't in the db yet. A top-level branch now lists all the wish's tasks. - The MCP server opened the db once at startup; a db created mid-session (fresh genie init) was never picked up (empty forever). Reopen the read-only handle per tool call when null. Existing dbs already saw writes via WAL; only the absent→created case was stale. Tests: hyphenated-slug top-level + group branches resolve correctly; a db created after the server started is picked up. check 631, build green.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the MCP server's branch resolution and database handling. It adds support for disambiguating hyphenated wish slugs against known wishes in the database and enables the server to dynamically pick up a database created mid-session. Feedback on these changes highlights a potential resource leak in src/term-commands/mcp.ts where dynamically opened database connections (ctx.db) are not closed by the server's shutdown handler. Additionally, it is recommended to simplify the SQL query in listWishSlugs by removing the redundant GROUP BY clause and wrapping the query in a try-catch block to handle uninitialized database states gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // 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); |
There was a problem hiding this comment.
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();
...
});| 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); | ||
| } |
There was a problem hiding this comment.
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.
| 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 []; | |
| } | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c60e1e39b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rest = branch.startsWith('wish/') ? branch.slice('wish/'.length) : null; | ||
| if (!rest) return null; | ||
| const known = db ? listWishSlugs(db) : []; // longest-first | ||
| if (known.includes(rest)) return { wish: rest, group: null }; |
There was a problem hiding this comment.
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 👍 / 👎.
| proc.stdin.write(`${JSON.stringify(INIT)}\n`); | ||
| seed(repo); // create .genie/genie.db + tasks AFTER the server opened (null) at startup |
There was a problem hiding this comment.
This test does not guarantee that the server has already opened a null DB handle before seed(repo) runs: the parent writes INIT and immediately creates the DB without waiting for the child to start and answer initialize. On a schedule where the child has not reached openReadonlyDb(cwd) yet, the DB exists at startup and the old one-open implementation would pass, so the regression test is not load-bearing. Wait for the initialize response first, then seed and send the tool call.
Useful? React with 👍 / 👎.
…lution Gemini HIGH: the per-call db reopen wrote to ctx.db while close() closed a separate startup 'db' local — a mid-session reopen leaked its handle. Drop the stale local; ctx.db is the single source of truth (close() closes the current handle). Codex P2: prefer a launch-worktree interpretation whose <group> is a REAL group of the prefix wish over a same-named top-level slug (disambiguates the genie-mcp / genie+mcp collision). Gemini MEDIUM: drop redundant GROUP BY (UNION already dedups). Codex P2: the reopen test now waits for the initialize reply (startup open ran) before seeding, so it can't pass without exercising the reopen. + collision test. check 632.
Addresses the two P2 findings from the Codex review on #2510/#2509 (the HIGH was already fixed in #2511).
P2a —
parseWishBranchmis-splits hyphenated wish slugswish/<slug>-<group>was split on the last-, so a top-levelwish/genie-mcpbranch resolved to ageniewish with anmcpgroup (returningresolved: truewith the wrong wish + empty tasks). Real launch worktrees likewish/genie-mcp-g2parsed correctly, so impact was limited to top-level wish branches (what the dogfood hit).Fix: disambiguate against the known wish slugs in the db (new
listWishSlugs, longest-first): exact match → top-level branch (groupnull); else the longest known slug that's a prefix followed by-<group>; the old last-dash heuristic only as a fallback when the wish has no tasks/groups yet. A top-level branch now lists all the wish's tasks (not group-filtered).P2b — MCP server never picks up a db created mid-session
The read-only db handle was opened once at startup. If the repo had no
genie.dbthen (null handle → empty board) and one was created during the session (a freshgenie init), the server served empty forever. Fix: reopen the read-only handle per tool call when it's null. (Existing dbs already saw new writes via WAL; only the absent→created case was stale.)Tests
wish/genie-mcp→{ wish: genie-mcp, group: null, resolved: true }(not the mis-splitgenie/mcp);wish/genie-mcp-g2→genie-mcp/g2.Both are load-bearing (fail on the old code). Gates:
bun run check631 pass; typecheck + build green.