Skip to content

Add PR creation action to pane git controls - #392

Open
Styrse wants to merge 5 commits into
dcouple:mainfrom
Styrse:pr
Open

Add PR creation action to pane git controls#392
Styrse wants to merge 5 commits into
dcouple:mainfrom
Styrse:pr

Conversation

@Styrse

@Styrse Styrse commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Description

Adds a PR action to worktree pane git controls. The action creates a GitHub pull request with gh pr create --fill, refreshes PR status, and opens the created PR URL. Also fixes a daemon IPC channel ownership mismatch for sessions:set-active-session.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING.md guidelines
  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have run pnpm typecheck and pnpm lint locally
  • I have tested the Electron app locally with pnpm electron-dev

Critical Areas Modified

  • State management/IPC events

Automated QA

Status: Passed at 8bed17ba.

  • PR action is disabled while the branch has an unpushed commit.
  • After a git-status refresh reports no unpushed commits, PR becomes enabled.
  • Clicking PR invokes the active session's create-PR IPC and opens the returned GitHub URL.
  • Full lint and typecheck pass; focused main tests pass 29/29; focused Playwright UI test passes 1/1.

Real GitHub PR creation was intentionally not executed against an external repository. Parsa may optionally verify that mutation with a disposable branch before merge.

@parsakhaz parsakhaz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verdict: Request changes. The create-PR path has two command-construction defects and malformed user-facing output.

The required .claude/agents/code-reviewer.md file is absent from this checkout, so the named skill could not be run under its own contract. I completed the correctness and security review directly using the available review schema.

Must Fix

  • MF-1: The handler resolves the PR base with getSessionComparisonBranch, which can intentionally return session.baseCommit for legacy panes (main/src/services/__tests__/worktreeManager.test.ts:224-240). That value is then sent to gh pr create --base (main/src/ipc/git.ts:1464-1476, main/src/services/worktreeManager.ts:1294-1300). GitHub CLI requires a branch name for --base, so affected legacy panes cannot create a PR. Use the local base-branch resolver intended for write targets and test the legacy-session case.

  • MF-2 (security): The new shell command escapes refs with host-platform rules (main/src/services/worktreeManager.ts:1294-1300). On Windows, escapeShellArg produces double-quoted arguments, but a WSL CommandRunner executes the result through Linux Bash. Git accepts refs such as refs/heads/$(touch-pwned) and backtick forms, and Bash expands those inside double quotes. Clicking PR in a WSL-backed repository can therefore execute command substitutions embedded in a crafted valid branch name. Use Bash-specific single-quote escaping whenever commandRunner.wslContext is set, and cover shell-sensitive refs in a unit test.

  • MF-3: The new start, success, and failure messages contain mojibake (main/src/ipc/git.ts:1466, main/src/ipc/git.ts:1480, main/src/ipc/git.ts:1505). Users receive ðŸ... and âœ... instead of the intended symbols. Replace them with the same Unicode strings used by adjacent git operations.

Should Fix

  • SF-1: No behavior test exercises createPullRequest; the only added test assertion checks channel registration (main/src/ipc/daemonRegistryBindings.test.ts:197). Add focused tests for command construction, URL extraction, WSL escaping, and error metadata. These tests are also needed to prove the security and base-branch fixes.

Security checks

  • Session and worktree existence are validated before process execution.
  • The URL opened in the renderer is derived from a GitHub-only regex in the main process.
  • No secrets are added to logs or IPC payloads.

@parsakhaz parsakhaz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-review at 667565d: all three Must-Fix findings are resolved. Full lint and typecheck pass; focused main tests pass 29/29; focused Playwright UI QA passes 1/1.

@parsakhaz

Copy link
Copy Markdown
Member

Review, simplify, refactor, and QA complete

The branch was rebased onto main and force-pushed with lease at 667565d7. I did not merge it.

Pass 1: Review

  • Fixed legacy panes passing a commit SHA to gh pr create --base; PR creation now resolves the local base branch intended for write operations.
  • Fixed a WSL command-injection path by selecting Bash escaping when the command runner targets WSL. Regression coverage includes valid Git refs containing command substitutions.
  • Repaired mojibake in start, success, and failure messages.
  • Updated the rebased handler for current JSON IPC payload rules and the current decoded git-error boundary.
  • Added tests for command construction, URL extraction, WSL escaping, and error metadata.

Review commit: f03a7a17 (fix(review): harden PR creation). The original request-changes findings are fixed.

Note: .claude/agents/code-reviewer.md, which the local code-reviewer skill requires, is absent from this repository. I disclosed that in the review and performed the correctness/security review directly with file and line evidence.

Pass 2: Simplify

  • Replaced repeated PR-button predicates and nested condition trees with one derived action state.
  • Typed the create-PR response at the preload boundary and removed the renderer's one-off runtime type guard.
  • Reused the existing decoded GitOperationError path instead of new error assertions.

Simplify commit: b35d3293 (refactor(simplify): streamline PR creation flow). Baseline lint had five PR-introduced blocking findings; post-pass lint has zero.

Pass 3: Refactor

  • Added shared/types/git.ts as the single create-PR result contract for main, preload, and renderer.
  • Added an automated UI flow and reusable Electron mock coverage for disabled, ready, IPC invocation, and external URL opening states.

Refactor commit: 667565d7 (refactor: share PR creation result contract).

Verification

  • pnpm typecheck: passed before and after simplification, and after refactor.
  • pnpm lint: improved from five blocking findings before simplification to a full pass after simplification and refactor, including Knip and boundary conformance.
  • pnpm --filter main exec vitest run src/services/__tests__/worktreeManager.test.ts src/ipc/daemonRegistryBindings.test.ts: 29/29 passed.
  • PLAYWRIGHT_PORT=45394 pnpm exec playwright test tests/review-availability.spec.ts -g "PR action requires pushed commits": 1/1 passed against a fresh worktree bundle.
  • UI assertion: one unpushed commit disables PR; a pushed status enables it; click invokes createPr('review-session') and opens https://github.com/dcouple/Pane/pull/392.
State Evidence
Unpushed commit, PR disabled PR action disabled while one commit remains unpushed
Pushed branch, PR enabled PR action enabled after branch status reports no unpushed commits

Follow-ups

  • Add handler-level registry tests before extracting the repeated git-mutation event, refresh, and error orchestration.
  • Introduce generic runtime decoding for typed IPC responses rather than solving it for only this method.
  • Add a fork-remote integration test before removing the explicit --head argument and relying on GitHub CLI inference.

Left for parsa

  • Let CI finish and review any external check failures.
  • Optionally perform one real authenticated gh pr create flow from Pane against a disposable branch. Automated QA mocked the external mutation intentionally.
  • Merge when satisfied. No merge was performed here.

@parsakhaz

Copy link
Copy Markdown
Member

Final verification addendum at 8bed17b

Windows CI exposed a platform-specific test assertion: native Windows correctly uses double-quoted shell arguments, while the test expected Unix single quotes. Commit 8bed17b makes the normalized-base behavior test deterministic without changing product behavior.

Current verification:

  • pnpm typecheck: passed
  • pnpm lint: passed, including Knip and boundary conformance
  • focused main tests: 29/29 passed
  • focused Playwright PR-action flow: 1/1 passed
  • origin/main is an ancestor of the pushed head

All three requested passes and their product changes remain as documented in the detailed QA comment. The branch is pushed to styrse/pr. No merge was performed.

Left for parsa: let the new CI run finish, optionally exercise one real authenticated PR creation on a disposable branch, then merge when satisfied.

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.

2 participants