Conversation
Branch created for: route api and mcp entry points through PR content resolution 🤖 Created with newpr
|
Claude finished @chrissena's task in 2m 19s —— View job PR Review —
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
=======================================
Coverage 81.76% 81.76%
=======================================
Files 97 97
Lines 20839 20839
Branches 4546 4546
=======================================
Hits 17040 17040
Misses 3799 3799 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Both createPr() call sites in src/api/create.ts embedded a hardcoded stub PR body and never accepted caller-supplied content, so the programmatic API and the MCP worktree_create_pr tool (which wraps it) still produced useless template PRs even after `wt new` gained --title/--body/--body-file support. - CreatePrOptions now accepts title/body/bodyFile; both createPr() branches call the existing resolvePRContent() resolver instead of building a literal, matching the CLI's flag -> AI -> template precedence and reporting the same titleSource/bodySource/aiProvider/ aiError provenance. Template text is byte-identical to before. - worktree_create_pr's input schema gains title/body/bodyFile, its description documents them, and its output schema documents the provenance fields; the handler threads the new args through. Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
The worktree_create_pr tests called the mocked createPr() directly with
hand-written args, so they only proved the test file could call a mock —
deleting title/body/bodyFile from the real createPr({...}) call inside
server.ts's request handler left all tests green. Since this PR exists
because one of three PR-creation entry points was fixed while two were
missed, tests that can't detect this class of regression recreate the
same failure mode one layer down.
- Extract the CallToolRequestSchema switch body into an exported
handleToolCall(name, args) function; the setRequestHandler callback
now just delegates to it. Purely mechanical — same body, same
behaviour, same response shape.
- Rewrite the worktree_create_pr handler tests to call handleToolCall(...)
directly (mocking only createPr), so they exercise the real dispatch
and arg-forwarding logic instead of restating it.
Verified: deleting title/body/bodyFile from the createPr({...}) call in
server.ts makes 2 of the new tests fail red; restoring it turns them
green again (both checked manually before this commit).
Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
|
Claude finished @chrissena's task in 5m 25s —— View job PR Review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: f215796c1a
ℹ️ 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".
ContentOverrides has five fields (title, body, bodyFile, forceAi, skipAi) and the CLI forwards all five, but the API's two createPr() overrides objects and the MCP worktree_create_pr tool forwarded only three — forceAi/skipAi were silently dropped, breaking the PR description's claim of "the same per-field precedence" as the CLI. - CreatePrOptions gains forceAi?/skipAi?, forwarded in both resolvePRContent() call sites in src/api/create.ts. - worktree_create_pr's input schema gains forceAi/skipAi, documented in the tool description, and threaded through the handler alongside title/body/bodyFile. - Tests prove the wiring: forceAi makes AI win over a supplied title, skipAi suppresses generatePRContentAsync entirely even when content is missing (both API-level and via the MCP handleToolCall path). Addresses reviewer thread A (4 threads) on PR #28. Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
createPr() pushed the new/current branch to origin, then resolved
caller-supplied --body/--body-file only afterwards. A bad combination
(both body and bodyFile) or an unreadable bodyFile surfaced as
INVALID_ARGUMENT only after the branch was already committed and
pushed — leaving an orphaned remote branch with no PR, and a retry
that then fails with BRANCH_EXISTS.
Hoist the validation (reusing readBodyOverride from pr-content.ts,
not a second reader) to the top of createPr(), before any git
mutation, and pass the already-resolved body string down to
resolvePRContent() so it is never re-read from disk.
Also fixes the `fs` mock in create.test.ts: pr-content.ts isn't
mocked in this file, so the real readBodyOverride() runs and calls
fs.readFileSync — missing from the vi.mock('fs') factory until now,
which the new test below depends on to simulate an unreadable file.
New test proves the fix: an unreadable bodyFile on the new-branch
path returns INVALID_ARGUMENT and git.push is never called.
Addresses reviewer thread B on PR #28 (codex P2). Also partially
addresses thread D (the fs.readFileSync mock half).
Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
The worktree_create_pr tool description claimed aiError is null "if not attempted or it succeeded", and the example response showed a template-sourced body with aiError: null. But resolvePRContent deliberately sets aiError to a descriptive string (e.g. "AI disabled (ai.provider = 'none')") whenever a field falls back to the template with the default, unconfigured AI provider — which is the common case for a caller who hasn't set up AI generation. An agent following the documented contract would misread that perfectly normal response as a failure. Rewrite the description bullet and the example to match pr-content.ts's actual behaviour: aiError is null only when AI wasn't needed at all or it succeeded, not just because the overall response looks normal. Addresses reviewer thread C on PR #28 (codex P2). Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
The existing-branch PR content resolution suite covers all four combinations of per-field precedence (nothing supplied, both supplied, title-only, body-only), but the new-branch suite was missing the "title from flag, body from AI" case — the mirror of the "body from flag, title from AI" test already there. Addresses the remaining half of reviewer thread D on PR #28 (the fs.readFileSync mock half shipped with the earlier validation-order fix, since that test depended on it). Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
|
Claude finished @chrissena's task in 6m 32s —— View job PR Review —
|
…ests Addresses PR review comments by claude. Both PRContentError throw sites live in readBodyOverride and both require `bodyFile`. Since d017cff hoisted that read to the top of createPr() and passes the contents as `body`, resolvePRContent can no longer raise it — so the two inner catches were unreachable. Removing them also removes a latent hazard the reviewer spotted: the new-branch handler restored actionResult.stashRef but silently abandoned unstagedStashRef, so had it ever become reachable it would have stranded the user's unstaged changes. The outer catch handles both. The three older MCP passthrough assertions omitted forceAi/skipAi. Vitest treats a missing key and an explicit `undefined` as equal, so dropping those fields from the handler would not have failed them — the same mutation-blindness this PR already fixed once. Now pinned. Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
|
Claude finished @chrissena's task in 3m 39s —— View job PR Review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfdccc479e
ℹ️ 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".
…tion Addresses PR review comments by chatgpt-codex-connector and claude. P1 — MCP stdout corruption. stdout on the MCP server process IS the JSON-RPC channel, and print()/printStatus() fall back to console.log unless JSON mode is on. Before this PR the MCP path never invoked AI, so printStatus was unreachable; routing PR creation through resolvePRContent makes it reachable — an AI-backed generation emits a status line on both success and failure, which would corrupt the protocol stream. main() now enables JSON mode before connecting. P2 — blank title. A whitespace-only title is a *defined* value, so it suppresses generation and reaches `gh pr create`, which rejects it — on the new-branch path only after the push, orphaning the branch just as an unreadable bodyFile did before d017cff hoisted that check. Now validated alongside the other overrides, before any git mutation. Also pins forceAi/skipAi in the last two MCP assertions; vitest treats a missing key and an explicit undefined as equal, so they could not have caught the handler dropping those fields. All four new tests mutation-verified: disabling either fix turns them red (2 each), restoring turns them green. Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
|
Claude finished @chrissena's task in 4m 16s —— View job PR Review —
|
Addresses PR review comment by claude. The existing-branch suite covered forceAi but not skipAi; the new-branch suite covered skipAi but not forceAi. Dropping the unmirrored key from either overrides object would have passed every test. Both suites now cover both flags. Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
|
Claude finished @chrissena's task in 4m 13s —— View job PR Review —
|
Addresses PR review comments by claude. The comment added when the dead PRContentError handler was removed claimed "the outer catch handles both" stash refs. It does not: the outer catch pops actionResult.stashRef only, so unstagedStashRef (stashed at 591, applied at 694) is never restored on an error path and any throw between those points strands the user's unstaged changes. The comment now names that gap as pre-existing, unfixed, and out of scope rather than implying it is handled — a comment asserting a safety property that does not hold is worse than no comment. The two mirror tests added last round asserted only source metadata, unlike every sibling in their suites, so a regression swapping title/body on those paths would have gone undetected. Both now pin the full github.createPr call shape. Claude-Session: https://claude.ai/code/session_01RRi2HJGVDvyPx4Bm7CvqPk
|
Claude finished @chrissena's task in 2m 42s —— View job PR Review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f52695dd
ℹ️ 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".
| // resolvePRContent() never re-reads the file. | ||
| let bodyOverride: string | undefined; | ||
| try { | ||
| bodyOverride = readBodyOverride({ body: bodyOverrideRaw, bodyFile: bodyFileOverride }); |
There was a problem hiding this comment.
When a programmatic caller sets cwd to a repository other than process.cwd() and supplies a relative bodyFile, this preflight reads the path relative to the process directory rather than the requested working directory. The call therefore returns INVALID_ARGUMENT (or reads an unrelated same-named file) even though the body file exists relative to cwd; resolve the file path against cwd before passing it to readBodyOverride.
Useful? React with 👍 / 👎.
| // top of this function and pass the contents as `body`, never `bodyFile`, | ||
| // so resolvePRContent cannot raise it. A catch that can never fire is | ||
| // dead code that only makes a future real error look handled. | ||
| const prContent: ResolvedPRContent = await resolvePRContent({ |
There was a problem hiding this comment.
When a configured AI provider succeeds or fails, this resolver reaches generatePRContentAsync, which calls printStatus and therefore writes to stdout while JSON mode is false. Direct consumers of the exported createPr API do not run the MCP server's setJsonMode(true), so both creation paths can unexpectedly pollute a host application's structured stdout. Fresh evidence beyond the earlier MCP issue is that the suppression fix exists only in src/mcp/server.ts startup; make content generation quiet for programmatic calls rather than relying on that server-global setting.
Useful? React with 👍 / 👎.
| const title = args?.title as string | undefined; | ||
| const body = args?.body as string | undefined; | ||
| const bodyFile = args?.bodyFile as string | undefined; | ||
| const forceAi = args?.forceAi as boolean | undefined; | ||
| const skipAi = args?.skipAi as boolean | undefined; |
There was a problem hiding this comment.
When a raw or buggy MCP client sends a non-string bodyFile, this assertion performs no runtime validation before the value reaches fs.readFileSync. In particular, JSON number 0 is treated by Node as the stdin file descriptor, so the server can block on or consume its own JSON-RPC input stream; non-string title or body values likewise produce UNKNOWN_ERROR failures at .trim(). Reject values whose runtime types do not match the advertised schema with INVALID_ARGUMENT before calling createPr.
Useful? React with 👍 / 👎.
| context: { | ||
| description: title, | ||
| branchName: currentBranch, | ||
| baseBranch, |
There was a problem hiding this comment.
When API or MCP PR creation uses a configured AI provider, this context omits the already-resolved repoRoot. generatePRContentAsync only gathers README and package metadata when context.repoRoot is present, so these newly enabled entry points always generate without the repository documentation that supplies project-specific terminology and conventions. Pass repoRoot at both resolver call sites so programmatic generation receives the same repository context the generation API supports.
Useful? React with 👍 / 👎.
1 participant
Summary
Part 1 taught
wt newto accept caller-supplied PR content, but it fixed onlyone of three entry points. The programmatic API (
src/api/create.ts) and theMCP
worktree_create_prtool still hardcode a stub body:So an agent driving the library or the MCP server — rather than the CLI —
still produces exactly the stub PRs this work set out to eliminate.
Changes
createPrcall sites insrc/api/create.tsthrough the samecontent resolution the CLI uses, instead of embedding a literal template.
worktree_create_prtool, with the same per-field precedence.Test Plan
api/create.tssites🤖 Opened with the
--title/--body-fileflags this PR extends.