SYMPHONY: Implement fork detection and integration in Symphony workflows#533
Open
chr1syy wants to merge 10 commits intoRunMaestro:mainfrom
Open
SYMPHONY: Implement fork detection and integration in Symphony workflows#533chr1syy wants to merge 10 commits intoRunMaestro:mainfrom
chr1syy wants to merge 10 commits intoRunMaestro:mainfrom
Conversation
Add isFork, forkSlug, and upstreamSlug fields to ActiveContribution interface. Create symphony-fork.ts utility that detects push access, forks repos via gh CLI, and reconfigures git remotes. Includes comprehensive test coverage for all paths. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Wire ensureForkSetup into all three Symphony contribution flows: - symphony:startContribution (new two-step flow) - symphony:start (older inline flow) - symphony:createDraftPR (deferred PR creation) Update createDraftPR to support cross-fork PRs by adding --repo and --head forkOwner:branch args when contributing via fork. Persist fork metadata (isFork, forkSlug, upstreamSlug) in both contribution state and metadata.json for later PR creation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Integrate ensureForkSetup into symphony-runner.ts so the standalone contribution service handles forked repos correctly: - startContribution: calls ensureForkSetup after clone/branch, returns isFork/forkSlug metadata, passes fork info to createDraftPR - createDraftPR: accepts upstreamSlug/forkOwner, adds --repo and --head flags for cross-fork PR creation - finalizeContribution: accepts upstreamSlug, adds --repo to gh pr ready/edit/view commands - cancelContribution: accepts upstreamSlug, adds --repo to gh pr close Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Update markPRReady and postPRComment to accept optional upstreamSlug and pass --repo flag to gh CLI for cross-fork PR operations - Thread fork metadata (isFork/forkSlug/upstreamSlug) through the symphony:complete handler to both functions - Sync fork fields from metadata.json to ActiveContribution state in checkPRStatuses and syncContribution handlers - Add structured logging at all fork decision points in both symphony.ts and symphony-runner.ts for workflow visibility Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a fork-setup utility and threads fork metadata (isFork, forkSlug, upstreamSlug, upstreamDefaultBranch) through IPC and runner flows; performs fork detection/setup during startContribution, pushes/targets forks when needed, and builds gh PR commands with appropriate --repo/--head handling for cross-fork PRs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Runner as Symphony Runner
participant Fork as Fork Utility (ensureForkSetup)
participant GH as GitHub CLI/API
participant Git as Git CLI
participant Handler as IPC Handler
User->>Runner: startContribution(repoPath, repoSlug)
Runner->>Fork: ensureForkSetup(repoPath, repoSlug)
Fork->>GH: gh api user (auth)
GH-->>Fork: user info
Fork->>GH: gh api repos/{repoSlug} (permissions)
GH-->>Fork: repo permissions
alt No push access
Fork->>GH: gh repo fork (create if needed)
GH-->>Fork: fork created/existing
Fork->>GH: gh api repos/{forkSlug} (clone URL)
GH-->>Fork: forkCloneUrl
Fork->>Git: git remote rename origin upstream (or set-url fallback)
Git-->>Fork: remote configured
Fork->>Git: git remote add origin {forkCloneUrl}
Git-->>Fork: origin set
Fork-->>Runner: {isFork: true, forkSlug, upstreamSlug, upstreamDefaultBranch}
else Has push access
Fork-->>Runner: {isFork: false}
end
alt Fork detected
Runner->>Handler: createDraftPR(..., upstreamSlug, forkOwner)
Handler->>GH: gh pr create --repo {upstreamSlug} --head {forkOwner}:{branch}
GH-->>Handler: prUrl
Handler-->>Runner: prUrl
else No fork
Runner->>Handler: createDraftPR(..., undefined, undefined)
Handler->>GH: gh pr create --head {branch}
GH-->>Handler: prUrl
Handler-->>Runner: prUrl
end
Runner-->>User: contribution started (includes isFork/forkSlug/upstreamSlug)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR introduces fork detection and end-to-end fork support for Symphony contribution workflows. A new Key changes:
One issue identified:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 42c649b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/main/services/symphony-runner.ts
Outdated
Comment on lines
+126
to
+136
| if (upstreamSlug && forkOwner) { | ||
| args.push('--repo', upstreamSlug); | ||
| // For cross-fork PRs, --head must specify the fork owner and branch | ||
| const branchResult = await execFileNoThrow( | ||
| 'git', | ||
| ['rev-parse', '--abbrev-ref', 'HEAD'], | ||
| localPath | ||
| ); | ||
| const branchName = branchResult.stdout.trim(); | ||
| args.push('--head', `${forkOwner}:${branchName}`); | ||
| } |
There was a problem hiding this comment.
--repo should be set independently of --head
Both the --repo and --head flags are currently gated on upstreamSlug && forkOwner. However, --repo should always be passed when upstreamSlug is set, regardless of whether forkOwner is available. If forkOwner is undefined while upstreamSlug is set (possible per the ForkSetupResult type), the --repo flag will be silently omitted and gh pr create will target the fork instead of the upstream repository.
This differs from the equivalent logic in src/main/ipc/handlers/symphony.ts, which correctly separates the two:
if (upstreamSlug) {
prArgs.push('--repo', upstreamSlug);
}Suggestion — split the guards so --repo is always set when upstreamSlug is provided:
Suggested change
| if (upstreamSlug && forkOwner) { | |
| args.push('--repo', upstreamSlug); | |
| // For cross-fork PRs, --head must specify the fork owner and branch | |
| const branchResult = await execFileNoThrow( | |
| 'git', | |
| ['rev-parse', '--abbrev-ref', 'HEAD'], | |
| localPath | |
| ); | |
| const branchName = branchResult.stdout.trim(); | |
| args.push('--head', `${forkOwner}:${branchName}`); | |
| } | |
| if (upstreamSlug) { | |
| args.push('--repo', upstreamSlug); | |
| } | |
| if (upstreamSlug && forkOwner) { | |
| // For cross-fork PRs, --head must specify the fork owner and branch | |
| const branchResult = await execFileNoThrow( | |
| 'git', | |
| ['rev-parse', '--abbrev-ref', 'HEAD'], | |
| localPath | |
| ); | |
| const branchName = branchResult.stdout.trim(); | |
| args.push('--head', `${forkOwner}:${branchName}`); | |
| } |
| .mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 }) // config user.email | ||
| .mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 }) // commit --allow-empty | ||
| .mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 }) // push | ||
| .mockResolvedValueOnce({ stdout: 'symphony/test-branch', stderr: '', exitCode: 0 }) // git rev-parse HEAD |
There was a problem hiding this comment.
Mock comment inaccuracy
The comment says git rev-parse HEAD but the actual call is git rev-parse --abbrev-ref HEAD (which gets the branch name, not the commit SHA). Since the mocks are positional, an inaccurate comment makes it harder to maintain this test.
Suggested change
| .mockResolvedValueOnce({ stdout: 'symphony/test-branch', stderr: '', exitCode: 0 }) // git rev-parse HEAD | |
| .mockResolvedValueOnce({ stdout: 'symphony/test-branch', stderr: '', exitCode: 0 }) // git rev-parse --abbrev-ref HEAD |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/ipc/handlers/symphony.ts (2)
2072-2077: Add validation when syncing fork metadata.The sync copies
metadata.forkSlugandmetadata.upstreamSlugwithout validating they exist. If metadata hasisFork: truebut missing slugs (due to a corrupted or partially written file), the contribution will have incomplete fork state.🛡️ Suggested defensive check
// Sync fork info from metadata to state -if (metadata.isFork && !contribution.isFork) { +if (metadata.isFork && metadata.upstreamSlug && !contribution.isFork) { contribution.isFork = metadata.isFork; contribution.forkSlug = metadata.forkSlug; contribution.upstreamSlug = metadata.upstreamSlug; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 2072 - 2077, The sync block that copies fork metadata (checking metadata.isFork and writing to contribution.isFork, contribution.forkSlug, contribution.upstreamSlug) must validate that metadata.forkSlug and metadata.upstreamSlug are present and non-empty before assigning; update the conditional in the sync logic to require metadata.isFork === true && metadata.forkSlug && metadata.upstreamSlug, and if either slug is missing avoid mutating contribution (or set a safe default) and emit a warning/log entry so corrupted/partial metadata doesn't produce incomplete fork state.
1784-1812: Potential edge case with legacy state files.The pattern
contribution.isFork ? contribution.upstreamSlug : undefinedassumes that ifisForkis true,upstreamSlugwill always be present. For newly created contributions this is guaranteed, but contributions deserialized from older state files (before this PR) could haveisFork: truewithoutupstreamSlug.The sync code at lines 2073-2077 partially mitigates this by syncing fork info from metadata files, but only when
metadata.isFork && !contribution.isFork. Consider adding defensive validation:const readyResult = await markPRReady( contribution.localPath, contribution.draftPrNumber, - contribution.isFork ? contribution.upstreamSlug : undefined + contribution.isFork && contribution.upstreamSlug ? contribution.upstreamSlug : undefined );This ensures that if
upstreamSlugis missing, the operation falls back to the default repository rather than passingundefinedexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 1784 - 1812, contribution.isFork may be true while contribution.upstreamSlug is missing for legacy state files; before calling markPRReady and postPRComment compute a safe upstreamSlug value (e.g., let upstream = contribution.isFork && contribution.upstreamSlug ? contribution.upstreamSlug : undefined) and pass that variable instead of the inline ternary, so markPRReady and postPRComment always receive either a valid upstreamSlug or undefined; update calls to markPRReady(contribution.localPath, contribution.draftPrNumber, upstream) and postPRComment(contribution.localPath, contribution.draftPrNumber, commentStats, upstream).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/symphony-runner.ts`:
- Around line 126-136: The git branch lookup using execFileNoThrow in
symphony-runner.ts can return a non-zero exitCode leaving branchName empty and
producing a malformed --head arg; update the branch retrieval logic (the call
assigned to branchResult and the derived branchName) to check
branchResult.exitCode and branchResult.stdout before using it, and if the
command fails or stdout is empty either throw or bail with a clear error (or
fallback to a safe default branch name) so you never push args.push('--head',
`${forkOwner}:${branchName}`) with an empty branch; ensure any thrown/returned
error is surfaced from the function that builds args.
- Around line 411-413: The current call builds closeArgs with '--delete-branch'
and may pass '--repo', upstreamSlug, then calls execFileNoThrow('gh', closeArgs,
localPath) which can silently fail to delete a forked head branch; change the
logic in the close/cleanup flow so that when upstreamSlug is present you do NOT
include '--delete-branch' (only call 'gh pr close' with the repo flag) and
instead either skip branch deletion or attempt an explicit, permission-aware
delete (e.g., only delete when the PR head owner matches the actor or after
checking maintainer-edit permission), and ensure failures from execFileNoThrow
are surfaced/logged (inspect closeResult stderr/status) so permission errors are
not ignored; update references: closeArgs, upstreamSlug, prNumber,
execFileNoThrow, localPath.
In `@src/main/utils/symphony-fork.ts`:
- Around line 88-117: After rewiring remotes in symphony-fork.ts the new origin
lacks refs/remotes/origin/HEAD so getDefaultBranch() can fall back to
main/master; update the logic around execFileNoThrow usage (referencing
renameResult, setUrlResult, addResult and the branch that adds origin) to run
"git remote set-head origin -a" after both the successful rename path (the else
branch where addResult is handled) and the set-url fallback path (where
setUrlResult is handled) to refresh origin/HEAD; also add a unit/integration
test that simulates a fork with a non-standard default branch (e.g., develop) to
assert getDefaultBranch() resolves the correct base after the remote rewrite.
---
Nitpick comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 2072-2077: The sync block that copies fork metadata (checking
metadata.isFork and writing to contribution.isFork, contribution.forkSlug,
contribution.upstreamSlug) must validate that metadata.forkSlug and
metadata.upstreamSlug are present and non-empty before assigning; update the
conditional in the sync logic to require metadata.isFork === true &&
metadata.forkSlug && metadata.upstreamSlug, and if either slug is missing avoid
mutating contribution (or set a safe default) and emit a warning/log entry so
corrupted/partial metadata doesn't produce incomplete fork state.
- Around line 1784-1812: contribution.isFork may be true while
contribution.upstreamSlug is missing for legacy state files; before calling
markPRReady and postPRComment compute a safe upstreamSlug value (e.g., let
upstream = contribution.isFork && contribution.upstreamSlug ?
contribution.upstreamSlug : undefined) and pass that variable instead of the
inline ternary, so markPRReady and postPRComment always receive either a valid
upstreamSlug or undefined; update calls to markPRReady(contribution.localPath,
contribution.draftPrNumber, upstream) and postPRComment(contribution.localPath,
contribution.draftPrNumber, commentStats, upstream).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ed36d8f-9fe8-4337-bb47-640a636890b4
📒 Files selected for processing (7)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/__tests__/main/services/symphony-runner.test.tssrc/__tests__/main/utils/symphony-fork.test.tssrc/main/ipc/handlers/symphony.tssrc/main/services/symphony-runner.tssrc/main/utils/symphony-fork.tssrc/shared/symphony-types.ts
- Split --repo and --head guards in createDraftPR so --repo is set independently when upstreamSlug is provided - Add branch name validation before building --head argument - Remove --delete-branch from cancelContribution for fork PRs to avoid cross-fork permission failures - Add git remote set-head origin -a after remote reconfiguration to ensure correct default branch resolution - Validate forkSlug and upstreamSlug exist before syncing fork metadata - Use safe upstreamSlug access in completion handler for legacy state - Fix test mock comments and update tests for all behavioral changes - Fix prettier formatting in all changed files Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/handlers/symphony.ts (1)
2056-2084:⚠️ Potential issue | 🟠 MajorFork metadata is persisted, but not reliably loaded back into
ActiveContribution.These sync paths only read
metadata.jsonwhendraftPrNumberis missing. In the session flow, state can already know the PR number while still missingisFork/forkSlug/upstreamSlug, sosymphony:completelater callsmarkPRReadyandpostPRCommentwithout--repofor fork PRs. Please merge the fork fields whenever they are absent, independently of PR-number recovery.Also applies to: 2250-2278, 2705-2709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 2056 - 2084, The metadata sync currently only applies fork fields when prCreated/draftPrNumber path runs; change the logic so fork info is merged independently: after parsing metadata (the JSON block that sets prCreated/draftPrNumber and prInfoSynced), always check if metadata.isFork && metadata.forkSlug && metadata.upstreamSlug and if contribution.isFork is falsy, then set contribution.isFork = metadata.isFork, contribution.forkSlug = metadata.forkSlug, contribution.upstreamSlug = metadata.upstreamSlug; apply the same independent-fork-merge change to the other two sync sites that handle metadata (the similar blocks around the code that sets draftPrNumber/prCreated/prInfoSynced at the other referenced locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 5224-5345: The tests under "fork support" miss the upstream-only
metadata case; add a new it(...) similar to the two existing tests that uses
createValidMetadata({ upstreamSlug: 'owner/repo' }) but no forkSlug, mocks
fs.readFile and execFileNoThrow the same way, calls getCreateDraftPRHandler()
with contributionId 'contrib_draft_test', asserts result.success is true, then
finds the gh pr create call (via
vi.mocked(execFileNoThrow).mock.calls.find(...)) and verifies that the args
include --repo owner/repo while the --head value is just the branch name (i.e.,
does not contain ':'), thereby covering the legacy/partial-metadata branch.
In `@src/__tests__/main/services/symphony-runner.test.ts`:
- Around line 1381-1387: The test currently only asserts ensureForkSetup was
called with the right args but not that it ran after branch creation; update the
test to assert call order by locating the mock for the branch checkout step
(e.g., the mocked function that performs "git checkout -b" — find the mock used
in this test suite such as checkoutBranch, gitExec, or similar) and add an
assertion that ensureForkSetup was invoked after that checkout mock (use
standard Jest invocation order, e.g. compare
ensureForkSetup.mock.invocationCallOrder[0] >
checkoutMock.mock.invocationCallOrder[0], or use toHaveBeenCalledAfter if your
test utils provide it) so the test enforces the ordering when calling
startContribution.
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 2605-2618: Call getDefaultBranch() to capture the upstream default
branch before calling ensureForkSetup(), because ensureForkSetup() rewrites
origin to point at the fork; specifically, invoke getDefaultBranch(localPath,
repoSlug) (or the existing getDefaultBranch call site) immediately before the
block that logs "Checking fork requirements" and stores its result in a variable
like upstreamDefaultBranch, then use upstreamDefaultBranch later when
constructing the PR --base value instead of relying on refs/remotes/origin/HEAD
after ensureForkSetup(); do not change ensureForkSetup() itself, just read and
store the upstream default branch prior to calling ensureForkSetup(localPath,
repoSlug).
In `@src/main/services/symphony-runner.ts`:
- Around line 416-423: The PR-close step builds closeArgs and calls
execFileNoThrow('gh', closeArgs, localPath) producing closeResult, but the code
currently logs errors and proceeds as if cancellation succeeded; change the flow
in the function that owns closeArgs/execFileNoThrow/closeResult (the PR
cancellation path) to check closeResult.success and when false immediately
return a failure result (e.g., { success: false } or throw) without deleting the
local checkout or continuing; ensure the existing log remains and include the
closeResult error/output in the returned failure context so the caller can
surface the cancellation failure to the user and retry cleanly.
---
Outside diff comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 2056-2084: The metadata sync currently only applies fork fields
when prCreated/draftPrNumber path runs; change the logic so fork info is merged
independently: after parsing metadata (the JSON block that sets
prCreated/draftPrNumber and prInfoSynced), always check if metadata.isFork &&
metadata.forkSlug && metadata.upstreamSlug and if contribution.isFork is falsy,
then set contribution.isFork = metadata.isFork, contribution.forkSlug =
metadata.forkSlug, contribution.upstreamSlug = metadata.upstreamSlug; apply the
same independent-fork-merge change to the other two sync sites that handle
metadata (the similar blocks around the code that sets
draftPrNumber/prCreated/prInfoSynced at the other referenced locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 109b412d-55ad-4527-9d99-468fbd837395
📒 Files selected for processing (6)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/__tests__/main/services/symphony-runner.test.tssrc/__tests__/main/utils/symphony-fork.test.tssrc/main/ipc/handlers/symphony.tssrc/main/services/symphony-runner.tssrc/main/utils/symphony-fork.ts
- Add upstream-only metadata test covering legacy/partial-metadata branch - Assert ensureForkSetup call order (after branch creation, not just args) - Capture upstream default branch before ensureForkSetup rewrites origin - Return failure from cancelContribution when gh pr close fails - Make fork info sync independent of draftPrNumber at both sync sites - Handle upstreamSlug in deferred PR creation without requiring isFork Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipc/handlers/symphony.ts (2)
1784-1814:⚠️ Potential issue | 🟠 MajorBackfill fork context before calling
markPRReady()andpostPRComment().In the
symphony:startContribution+registerActive()flow,ActiveContributionstill is not populated withisFork/upstreamSlug. If the user completes before a sync pass repairs state, this block runs without the upstream repo context required for a cross-fork PR. Either persist those fields intostate.activewhen the contribution is registered/updated, or load them frommetadata.jsonhere before invoking the GH commands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 1784 - 1814, The ActiveContribution may lack isFork/upstreamSlug when this code runs; before calling markPRReady and postPRComment, backfill fork context into contribution by loading the repo metadata (e.g., reading metadata.json under contribution.localPath or using your existing metadata loader) and set contribution.isFork and contribution.upstreamSlug accordingly (or derive upstreamSlug into a local upstreamSlug variable if absent on contribution) so the calls to markPRReady(...) and postPRComment(...) always get correct upstreamFork context; ensure this backfill happens in the symphony:startContribution/registerActive flow or immediately prior to invoking markPRReady and postPRComment.
2242-2280:⚠️ Potential issue | 🟠 MajorCarry
forkSluginto PR recovery.This step now restores
forkSlug, but the recovery step below still callsdiscoverPRByBranch(contribution.repoSlug, contribution.branchName). For fork contributions that lostdraftPrNumber, that lookup still searches with the upstream owner instead offorkOwner:branch, so the existing cross-fork PR is never rediscovered. The same fix is needed incheckPRStatuses().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 2242 - 2280, The PR recovery misses cross-fork lookups because discoverPRByBranch(contribution.repoSlug, contribution.branchName) always uses the upstream repo owner; when you restore contribution.forkSlug you must call discoverPRByBranch with the fork owner instead of contribution.repoSlug (e.g., use contribution.forkSlug or construct the fork repo slug) so it searches forkOwner:branch; update the same logic inside checkPRStatuses() to prefer contribution.forkSlug when contribution.isFork/forkSlug is present before calling discoverPRByBranch.
♻️ Duplicate comments (1)
src/main/ipc/handlers/symphony.ts (1)
2948-2963:⚠️ Potential issue | 🟠 MajorReuse the upstream base branch in the deferred PR path.
This handler now targets
metadata.upstreamSlug, butbaseBranchwas still resolved earlier from the currentorigin. AfterensureForkSetup()repointsoriginat the fork, a fork with a different default branch can make the deferredrev-list/ PR creation flow use the fork’s default instead of the upstream base. Persist the pre-fork branch fromsymphony:startContribution()metadata and use it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 2948 - 2963, The deferred PR flow still uses the local variable baseBranch which may have been repointed to the fork; replace that with the pre-fork/upstream base branch persisted in the startContribution metadata (e.g. metadata.preForkBaseBranch or metadata.upstreamBaseBranch) when calling createDraftPR: compute a baseBranchToUse = metadata.preForkBaseBranch ?? baseBranch and pass baseBranchToUse to createDraftPR (keep metaForkOwner and other args unchanged) so the PR uses the original upstream base branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 5224-5407: The tests only assert gh pr create flags but not the
preceding git push behavior; update each relevant test (those using
getCreateDraftPRHandler and mocking execFileNoThrow) to also find the git push
execFileNoThrow mock call (search mock.calls for call[0] === 'git' &&
call[1]?.[0] === 'push') and assert its args follow the fork-aware path: when
metadata.isFork / forkSlug is present, ensure the push refspec or remote
includes the fork owner (e.g., a refspec containing "chris:" or remote target
matching the fork), and when no fork/forkSlug or only upstreamSlug is present,
assert the push args do not include a fork owner prefix (no ':' in the pushed
refspec); use the same execFileNoThrow mock call inspection pattern as the
existing prCall assertions.
- Around line 2131-2155: The test asserts ensureForkSetup was called with
correct args but not that it ran after branch creation; update the test using
the mocks for execFileNoThrow and ensureForkSetup (and the getStartHandler
invocation) to assert call order: identify the execFileNoThrow call that
performs the git checkout/-b (check args.includes('checkout') or args[0] ===
'checkout') and assert its mock invocation occurs before ensureForkSetup's
invocation (e.g., compare mock.invocationCallOrder entries or compare positions
in mock.calls), and apply the same ordering assertion to the other similar test
covering the same flow where ensureForkSetup must run after branch creation.
---
Outside diff comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1784-1814: The ActiveContribution may lack isFork/upstreamSlug
when this code runs; before calling markPRReady and postPRComment, backfill fork
context into contribution by loading the repo metadata (e.g., reading
metadata.json under contribution.localPath or using your existing metadata
loader) and set contribution.isFork and contribution.upstreamSlug accordingly
(or derive upstreamSlug into a local upstreamSlug variable if absent on
contribution) so the calls to markPRReady(...) and postPRComment(...) always get
correct upstreamFork context; ensure this backfill happens in the
symphony:startContribution/registerActive flow or immediately prior to invoking
markPRReady and postPRComment.
- Around line 2242-2280: The PR recovery misses cross-fork lookups because
discoverPRByBranch(contribution.repoSlug, contribution.branchName) always uses
the upstream repo owner; when you restore contribution.forkSlug you must call
discoverPRByBranch with the fork owner instead of contribution.repoSlug (e.g.,
use contribution.forkSlug or construct the fork repo slug) so it searches
forkOwner:branch; update the same logic inside checkPRStatuses() to prefer
contribution.forkSlug when contribution.isFork/forkSlug is present before
calling discoverPRByBranch.
---
Duplicate comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 2948-2963: The deferred PR flow still uses the local variable
baseBranch which may have been repointed to the fork; replace that with the
pre-fork/upstream base branch persisted in the startContribution metadata (e.g.
metadata.preForkBaseBranch or metadata.upstreamBaseBranch) when calling
createDraftPR: compute a baseBranchToUse = metadata.preForkBaseBranch ??
baseBranch and pass baseBranchToUse to createDraftPR (keep metaForkOwner and
other args unchanged) so the PR uses the original upstream base branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31e0978a-4b28-4a06-8022-d3df66f169bf
📒 Files selected for processing (4)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/__tests__/main/services/symphony-runner.test.tssrc/main/ipc/handlers/symphony.tssrc/main/services/symphony-runner.ts
- Fix discoverPRByBranch to use fork owner as head ref for cross-fork PRs - Persist upstreamDefaultBranch in metadata so deferred PR creation uses correct base branch after fork setup reconfigures origin - Add call order assertion: branch creation before ensureForkSetup Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/__tests__/main/ipc/handlers/symphony.test.ts (2)
4654-4682:⚠️ Potential issue | 🟡 MinorAssert the call ordering here too.
This test only proves
ensureForkSetup()was called, not that it ran aftergit checkout -bas the name promises. Reuse the invocation-order check from the siblingsymphony:startfork test so this suite catches a reordered call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/symphony.test.ts` around lines 4654 - 4682, The test only checks ensureForkSetup was called, not that it ran after the git checkout; update the test that uses getStartContributionHandler, vi.mocked(execFileNoThrow) and vi.mocked(ensureForkSetup) to also assert call ordering: capture the invocation order of the execFileNoThrow call for the 'git' 'checkout' branch creation and compare it to ensureForkSetup's invocation order (ensureForkSetup's invocation index is greater), so the test verifies ensureForkSetup ran after the checkout; reference the mocked functions execFileNoThrow, ensureForkSetup and the handler from getStartContributionHandler and use their mock invocation metadata to perform the ordering assertion.
5237-5419:⚠️ Potential issue | 🟠 MajorAlso assert the fork-aware
git pushpath.These cases only inspect
gh pr createargs. The Linux regression from#531happens earlier if the branch is still pushed to upstream, so the suite can stay green while the real failure is back. Check the precedinggit pushcall targets the fork whenforkSlugexists and stays unprefixed otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/symphony.test.ts` around lines 5237 - 5419, Add assertions that verify the fork-aware git push call was made: find the mocked execFileNoThrow call where call[0] === 'git' and call[1]?[0] === 'push' and assert its args target the fork when metadata.forkSlug is present (metadata created with forkSlug: 'chris/repo' — expect the push args to include the fork owner prefix 'chris' or a ref like 'chris:branchName'), and assert it does not include that fork prefix when metadata has no forkSlug (or only upstreamSlug); update the three tests (the ones using createValidMetadata with fork info, without fork info, and with only upstreamSlug) to locate the push call via vi.mocked(execFileNoThrow).mock.calls and add the corresponding expectation on the push call arguments to ensure push targets the fork in the fork case and the plain branch/remote in the non-fork/upstream-only cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 2190-2225: The test "should persist fork info in contribution when
fork is needed" currently verifies isFork, forkSlug, and upstreamSlug but omits
upstreamDefaultBranch; update the test fixtures and mocks so the fork flow
includes and persists upstreamDefaultBranch (e.g., have
vi.mocked(ensureForkSetup) resolve with upstreamDefaultBranch set to a value
different from origin/HEAD) then assert savedContrib.upstreamDefaultBranch
equals that value; ensure any other fork-related tests (the ones noted) receive
the same upstreamDefaultBranch in ensureForkSetup and include assertions that
the persisted state (from fs.writeFile inspected via writeStateCall) contains
upstreamDefaultBranch so the deferred gh pr create logic can rely on it.
---
Duplicate comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 4654-4682: The test only checks ensureForkSetup was called, not
that it ran after the git checkout; update the test that uses
getStartContributionHandler, vi.mocked(execFileNoThrow) and
vi.mocked(ensureForkSetup) to also assert call ordering: capture the invocation
order of the execFileNoThrow call for the 'git' 'checkout' branch creation and
compare it to ensureForkSetup's invocation order (ensureForkSetup's invocation
index is greater), so the test verifies ensureForkSetup ran after the checkout;
reference the mocked functions execFileNoThrow, ensureForkSetup and the handler
from getStartContributionHandler and use their mock invocation metadata to
perform the ordering assertion.
- Around line 5237-5419: Add assertions that verify the fork-aware git push call
was made: find the mocked execFileNoThrow call where call[0] === 'git' and
call[1]?[0] === 'push' and assert its args target the fork when
metadata.forkSlug is present (metadata created with forkSlug: 'chris/repo' —
expect the push args to include the fork owner prefix 'chris' or a ref like
'chris:branchName'), and assert it does not include that fork prefix when
metadata has no forkSlug (or only upstreamSlug); update the three tests (the
ones using createValidMetadata with fork info, without fork info, and with only
upstreamSlug) to locate the push call via vi.mocked(execFileNoThrow).mock.calls
and add the corresponding expectation on the push call arguments to ensure push
targets the fork in the fork case and the plain branch/remote in the
non-fork/upstream-only cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe0335fd-8299-47ca-98f7-7738bc8fcb79
📒 Files selected for processing (2)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/main/ipc/handlers/symphony.ts
…call ordering - Assert upstreamDefaultBranch is persisted in startContribution fork metadata - Verify ensureForkSetup runs after git checkout using invocationCallOrder - Assert deferred createDraftPR uses persisted upstreamDefaultBranch as --base Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/__tests__/main/ipc/handlers/symphony.test.ts (1)
2227-2265:⚠️ Potential issue | 🟠 MajorAssert the fork-aware
git pushpath too.These cases still only prove the
gh pr createflags. Issue#531fails earlier if the branch is pushed to upstream instead of the fork, so the regression can come back while these tests stay green. Please inspect the precedinggit pushcall and verify it targets the fork whenforkSlugis present, and the non-fork path otherwise.Also applies to: 5249-5436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/symphony.test.ts` around lines 2227 - 2265, Add assertions that the mocked git push targets the fork when ensureForkSetup returns isFork:true and targets upstream when not a fork: after calling handler, find the push call from vi.mocked(execFileNoThrow).mock.calls where call[0] === 'git' and call[1]?.[0] === 'push' and assert its args include the fork remote/name derived from forkSlug (e.g., 'chris' or a 'chris:branch' ref) when forkSlug is 'chris/repo'; also add a parallel test or branch check that when ensureForkSetup resolves { isFork: false } the push args target the upstream remote (e.g., 'origin' or no fork-prefixed ref). Ensure you reference the mocked execFileNoThrow calls and the ensureForkSetup mock used in the test (and reuse getStartHandler/validStartParams) so the push behavior is verified alongside the gh pr create checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 5318-5373: The test's check of the GitHub CLI `--head` behavior is
unsafe because it uses headIdx = prArgs.indexOf('--head') and then reads
prArgs[headIdx + 1] without ensuring headIdx !== -1; update the assertions in
the test that calls getCreateDraftPRHandler() (and the similar block around the
other test) to first assert that '--head' exists
(expect(headIdx).toBeGreaterThanOrEqual(0)) before validating the following
value, or—if the intended contract is that `--head` must be omitted for non-fork
metadata—assert that prArgs does not contain '--head'
(expect(prArgs).not.toContain('--head')) instead; adjust the checks near the
mocked execFileNoThrow/prCall/prArgs/headIdx references accordingly so they
reliably verify presence/absence and the branch-name format for `--head`.
---
Duplicate comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 2227-2265: Add assertions that the mocked git push targets the
fork when ensureForkSetup returns isFork:true and targets upstream when not a
fork: after calling handler, find the push call from
vi.mocked(execFileNoThrow).mock.calls where call[0] === 'git' and call[1]?.[0]
=== 'push' and assert its args include the fork remote/name derived from
forkSlug (e.g., 'chris' or a 'chris:branch' ref) when forkSlug is 'chris/repo';
also add a parallel test or branch check that when ensureForkSetup resolves {
isFork: false } the push args target the upstream remote (e.g., 'origin' or no
fork-prefixed ref). Ensure you reference the mocked execFileNoThrow calls and
the ensureForkSetup mock used in the test (and reuse
getStartHandler/validStartParams) so the push behavior is verified alongside the
gh pr create checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a8f7c9e-9cd3-4002-b824-aacb62aa8966
📒 Files selected for processing (1)
src/__tests__/main/ipc/handlers/symphony.test.ts
…k tests Ensure headIdx, repoIdx, and baseIdx are asserted >= 0 before accessing the following array element, preventing silent false-passes if the flag were ever missing from gh pr create args. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
Author
|
@coderabbitai re-review please |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/symphony-runner.ts (1)
396-403:⚠️ Potential issue | 🟠 MajorCheck
gh pr viewbefore returning success.If
gh pr viewexits non-zero, this path still returnssuccess: truewith an emptyprUrl, so the caller treats finalization as complete even though the upstream PR lookup failed.Suggested fix
const prInfoResult = await execFileNoThrow('gh', viewArgs, localPath); +if (prInfoResult.exitCode !== 0) { + return { success: false, error: `Failed to fetch PR URL: ${prInfoResult.stderr}` }; +} return { success: true, prUrl: prInfoResult.stdout.trim(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/symphony-runner.ts` around lines 396 - 403, The current return always sets success: true even when execFileNoThrow('gh', viewArgs, localPath) fails; update the logic that builds the response after calling execFileNoThrow (symbols: viewArgs, execFileNoThrow, prInfoResult, prUrl, success) to check prInfoResult.exitCode/ code (or prInfoResult.stderr) and that prInfoResult.stdout.trim() is non-empty before returning success: true; if the gh command failed or produced empty output, return success: false and include the error/stderr (or a clear message) instead of an empty prUrl so callers know the PR lookup failed.
♻️ Duplicate comments (1)
src/__tests__/main/ipc/handlers/symphony.test.ts (1)
2227-2266:⚠️ Potential issue | 🟠 MajorAssert the fork-aware
git pushpath in the fork PR tests.These cases only lock in
gh pr createflags. Issue#531breaks earlier when the branch is pushed to a remote the user cannot write to, so the regression can come back while these tests stay green.What to assert
+const pushCall = vi.mocked(execFileNoThrow).mock.calls.find( + (call) => call[0] === 'git' && call[1]?.[0] === 'push' +); +expect(pushCall).toBeDefined(); +// fork case: assert the push targets the fork-aware remote/refspec +// non-fork case: assert no fork-owner prefix is presentAlso applies to: 5250-5441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/symphony.test.ts` around lines 2227 - 2266, The test only asserts gh pr create flags but not that the branch was pushed to the fork remote; add assertions that execFileNoThrow was invoked for the git push path and that the push targeted the fork owner from ensureForkSetup (forkSlug 'chris/...') rather than the upstream remote. Specifically, after calling getStartHandler() check vi.mocked(execFileNoThrow).mock.calls for a call where call[0] === 'git' and call[1]?.[0] === 'push' and assert the push args include the fork remote or ref containing 'chris' and the branch name returned by the mocked 'git rev-parse' ('symphony/issue-42-abc'), so the test verifies the fork-aware git push behavior (references: getStartHandler, execFileNoThrow, ensureForkSetup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 2720-2725: The fork metadata (forkResult.isFork,
forkResult.forkSlug, repoSlug/upstreamSlug) is only being written to
metadata.json but not persisted into the in-memory state used by later handlers;
update the code paths that currently spread fork fields into the metadata object
(the block using upstreamDefaultBranch, isFork: forkResult.isFork,
...(forkResult.isFork && { forkSlug: forkResult.forkSlug, upstreamSlug: repoSlug
})) to also write the same fields into state.active.contribution (e.g. set
state.active.contribution.isFork = forkResult.isFork and when true set
state.active.contribution.forkSlug = forkResult.forkSlug and
state.active.contribution.upstreamSlug = repoSlug) so symphony:complete can read
them immediately; apply this change to the other similar spots mentioned (the
blocks around the other diffs at 2775-2782 and 2828-2834) and ensure any state
persistence function that writes metadata.json is accompanied by the
corresponding update to state.active before returning.
In `@src/main/utils/symphony-fork.ts`:
- Around line 88-120: The git commands that rewrite remotes and set HEAD (the
execFileNoThrow invocations for ['remote','rename','origin','upstream'],
['remote','set-url','origin',forkCloneUrl],
['remote','add','origin',forkCloneUrl'] and ['remote','set-head','origin','-a'])
are called without the expanded environment; update each execFileNoThrow call in
symphony-fork.ts to include the expanded env from getExpandedEnv() (pass it into
the execFileNoThrow options/env parameter) so those git child processes use the
same environment as the earlier gh calls and avoid resolution failures.
- Around line 93-106: The rename fallback currently runs for any failure of the
git remote rename operation; update the logic around renameResult (the result of
the git remote rename origin upstream executed via execFileNoThrow) to only
attempt the set-url fallback when stderr or exitCode indicates the specific
"remote upstream already exists" error (or an equivalent recognizable message),
otherwise surface/log the original error and return failure; keep using
logger.warn/error, still reference forkCloneUrl and repoPath for the set-url
call when the specific condition matches, and do not rewrite origin for
unrelated rename failures.
- Around line 42-51: The probe that checks push permission using
execFileNoThrow('gh', ['api', `repos/${repoSlug}`, '--jq', '.permissions.push'],
...) currently treats non-zero exit codes as "no push access" and continues to
the fork path; change this to fail fast instead: in the code that inspects
accessResult (the variable returned from execFileNoThrow), if
accessResult.exitCode !== 0 log an error with the accessResult.stdout/stderr and
either throw an Error or return a failing result rather than proceeding to
create/rewire a fork (update the block that currently checks
accessResult.exitCode === 0 && accessResult.stdout.trim() === 'true' to handle
the non-zero case explicitly), keeping logger.info(LOG_CONTEXT) usages for the
successful branch.
---
Outside diff comments:
In `@src/main/services/symphony-runner.ts`:
- Around line 396-403: The current return always sets success: true even when
execFileNoThrow('gh', viewArgs, localPath) fails; update the logic that builds
the response after calling execFileNoThrow (symbols: viewArgs, execFileNoThrow,
prInfoResult, prUrl, success) to check prInfoResult.exitCode/ code (or
prInfoResult.stderr) and that prInfoResult.stdout.trim() is non-empty before
returning success: true; if the gh command failed or produced empty output,
return success: false and include the error/stderr (or a clear message) instead
of an empty prUrl so callers know the PR lookup failed.
---
Duplicate comments:
In `@src/__tests__/main/ipc/handlers/symphony.test.ts`:
- Around line 2227-2266: The test only asserts gh pr create flags but not that
the branch was pushed to the fork remote; add assertions that execFileNoThrow
was invoked for the git push path and that the push targeted the fork owner from
ensureForkSetup (forkSlug 'chris/...') rather than the upstream remote.
Specifically, after calling getStartHandler() check
vi.mocked(execFileNoThrow).mock.calls for a call where call[0] === 'git' and
call[1]?.[0] === 'push' and assert the push args include the fork remote or ref
containing 'chris' and the branch name returned by the mocked 'git rev-parse'
('symphony/issue-42-abc'), so the test verifies the fork-aware git push behavior
(references: getStartHandler, execFileNoThrow, ensureForkSetup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0db1d59c-f90f-41cf-8e70-879858e3f3b1
📒 Files selected for processing (7)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/__tests__/main/services/symphony-runner.test.tssrc/__tests__/main/utils/symphony-fork.test.tssrc/main/ipc/handlers/symphony.tssrc/main/services/symphony-runner.tssrc/main/utils/symphony-fork.tssrc/shared/symphony-types.ts
Comment on lines
+2720
to
+2725
| upstreamDefaultBranch, | ||
| isFork: forkResult.isFork, | ||
| ...(forkResult.isFork && { | ||
| forkSlug: forkResult.forkSlug, | ||
| upstreamSlug: repoSlug, | ||
| }), |
There was a problem hiding this comment.
Persist fork metadata into state.active, not just metadata.json.
In this workflow the fork fields are only written to metadata.json. symphony:complete later reads contribution.isFork / contribution.upstreamSlug from state.active to decide whether to add --repo, so a fork contribution can still be completed against the fork checkout until checkPRStatuses or syncContribution happens to backfill state.
Suggested fix
await fs.writeFile(
metadataPath,
JSON.stringify(
{
@@
2
)
);
@@
if (prResult.success) {
draftPrNumber = prResult.prNumber;
draftPrUrl = prResult.prUrl;
@@
await fs.writeFile(metadataPath, JSON.stringify(metaContent, null, 2));
}
}
+
+ const state = await readState(app);
+ const activeContribution = state.active.find((c) => c.id === contributionId);
+ if (activeContribution) {
+ activeContribution.isFork = forkResult.isFork;
+ if (forkResult.isFork) {
+ activeContribution.forkSlug = forkResult.forkSlug;
+ activeContribution.upstreamSlug = repoSlug;
+ }
+ activeContribution.draftPrNumber = draftPrNumber;
+ activeContribution.draftPrUrl = draftPrUrl;
+ await writeState(app, state);
+ }
// 6. Broadcast status update
const mainWindow = getMainWindow?.();Also applies to: 2775-2782, 2828-2834
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/symphony.ts` around lines 2720 - 2725, The fork
metadata (forkResult.isFork, forkResult.forkSlug, repoSlug/upstreamSlug) is only
being written to metadata.json but not persisted into the in-memory state used
by later handlers; update the code paths that currently spread fork fields into
the metadata object (the block using upstreamDefaultBranch, isFork:
forkResult.isFork, ...(forkResult.isFork && { forkSlug: forkResult.forkSlug,
upstreamSlug: repoSlug })) to also write the same fields into
state.active.contribution (e.g. set state.active.contribution.isFork =
forkResult.isFork and when true set state.active.contribution.forkSlug =
forkResult.forkSlug and state.active.contribution.upstreamSlug = repoSlug) so
symphony:complete can read them immediately; apply this change to the other
similar spots mentioned (the blocks around the other diffs at 2775-2782 and
2828-2834) and ensure any state persistence function that writes metadata.json
is accompanied by the corresponding update to state.active before returning.
Comment on lines
+42
to
+51
| const accessResult = await execFileNoThrow( | ||
| 'gh', | ||
| ['api', `repos/${repoSlug}`, '--jq', '.permissions.push'], | ||
| undefined, | ||
| env | ||
| ); | ||
| if (accessResult.exitCode === 0 && accessResult.stdout.trim() === 'true') { | ||
| logger.info('User has push access, no fork needed', LOG_CONTEXT); | ||
| return { isFork: false }; | ||
| } |
There was a problem hiding this comment.
Fail fast when the push-permission probe fails.
A non-zero gh api repos/${repoSlug} result currently falls through into the fork path. That can create or rewire a fork because of an unrelated API/auth/network failure instead of because push access was actually denied.
Possible fix
const accessResult = await execFileNoThrow(
'gh',
['api', `repos/${repoSlug}`, '--jq', '.permissions.push'],
undefined,
env
);
- if (accessResult.exitCode === 0 && accessResult.stdout.trim() === 'true') {
+ if (accessResult.exitCode !== 0) {
+ logger.error('Failed to check push access', LOG_CONTEXT, { stderr: accessResult.stderr });
+ return { isFork: false, error: `Failed to check push access: ${accessResult.stderr}` };
+ }
+ if (accessResult.stdout.trim() === 'true') {
logger.info('User has push access, no fork needed', LOG_CONTEXT);
return { isFork: false };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const accessResult = await execFileNoThrow( | |
| 'gh', | |
| ['api', `repos/${repoSlug}`, '--jq', '.permissions.push'], | |
| undefined, | |
| env | |
| ); | |
| if (accessResult.exitCode === 0 && accessResult.stdout.trim() === 'true') { | |
| logger.info('User has push access, no fork needed', LOG_CONTEXT); | |
| return { isFork: false }; | |
| } | |
| const accessResult = await execFileNoThrow( | |
| 'gh', | |
| ['api', `repos/${repoSlug}`, '--jq', '.permissions.push'], | |
| undefined, | |
| env | |
| ); | |
| if (accessResult.exitCode !== 0) { | |
| logger.error('Failed to check push access', LOG_CONTEXT, { stderr: accessResult.stderr }); | |
| return { isFork: false, error: `Failed to check push access: ${accessResult.stderr}` }; | |
| } | |
| if (accessResult.stdout.trim() === 'true') { | |
| logger.info('User has push access, no fork needed', LOG_CONTEXT); | |
| return { isFork: false }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/utils/symphony-fork.ts` around lines 42 - 51, The probe that checks
push permission using execFileNoThrow('gh', ['api', `repos/${repoSlug}`, '--jq',
'.permissions.push'], ...) currently treats non-zero exit codes as "no push
access" and continues to the fork path; change this to fail fast instead: in the
code that inspects accessResult (the variable returned from execFileNoThrow), if
accessResult.exitCode !== 0 log an error with the accessResult.stdout/stderr and
either throw an Error or return a failing result rather than proceeding to
create/rewire a fork (update the block that currently checks
accessResult.exitCode === 0 && accessResult.stdout.trim() === 'true' to handle
the non-zero case explicitly), keeping logger.info(LOG_CONTEXT) usages for the
successful branch.
Comment on lines
+88
to
+120
| const renameResult = await execFileNoThrow( | ||
| 'git', | ||
| ['remote', 'rename', 'origin', 'upstream'], | ||
| repoPath | ||
| ); | ||
| if (renameResult.exitCode !== 0) { | ||
| // Fallback: upstream already exists, just set origin URL | ||
| logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, { | ||
| stderr: renameResult.stderr, | ||
| }); | ||
| const setUrlResult = await execFileNoThrow( | ||
| 'git', | ||
| ['remote', 'set-url', 'origin', forkCloneUrl], | ||
| repoPath | ||
| ); | ||
| if (setUrlResult.exitCode !== 0) { | ||
| logger.error('Failed to set origin URL', LOG_CONTEXT, { stderr: setUrlResult.stderr }); | ||
| return { isFork: false, error: `Failed to reconfigure remotes: ${setUrlResult.stderr}` }; | ||
| } | ||
| } else { | ||
| const addResult = await execFileNoThrow( | ||
| 'git', | ||
| ['remote', 'add', 'origin', forkCloneUrl], | ||
| repoPath | ||
| ); | ||
| if (addResult.exitCode !== 0) { | ||
| logger.error('Failed to add origin remote', LOG_CONTEXT, { stderr: addResult.stderr }); | ||
| return { isFork: false, error: `Failed to add origin remote: ${addResult.stderr}` }; | ||
| } | ||
| } | ||
|
|
||
| // Set HEAD for origin so getDefaultBranch() works correctly with fork remotes | ||
| await execFileNoThrow('git', ['remote', 'set-head', 'origin', '-a'], repoPath); |
There was a problem hiding this comment.
Pass the expanded env to the git calls too.
getExpandedEnv() is prepared up front, but the remote rewrite and set-head commands run without it. In packaged Electron installs that can make fork setup fail with git resolution errors even though the earlier gh calls succeeded.
Possible fix
const renameResult = await execFileNoThrow(
'git',
['remote', 'rename', 'origin', 'upstream'],
- repoPath
+ repoPath,
+ env
);
@@
const setUrlResult = await execFileNoThrow(
'git',
['remote', 'set-url', 'origin', forkCloneUrl],
- repoPath
+ repoPath,
+ env
);
@@
const addResult = await execFileNoThrow(
'git',
['remote', 'add', 'origin', forkCloneUrl],
- repoPath
+ repoPath,
+ env
);
@@
- await execFileNoThrow('git', ['remote', 'set-head', 'origin', '-a'], repoPath);
+ await execFileNoThrow('git', ['remote', 'set-head', 'origin', '-a'], repoPath, env);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const renameResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'rename', 'origin', 'upstream'], | |
| repoPath | |
| ); | |
| if (renameResult.exitCode !== 0) { | |
| // Fallback: upstream already exists, just set origin URL | |
| logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, { | |
| stderr: renameResult.stderr, | |
| }); | |
| const setUrlResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'set-url', 'origin', forkCloneUrl], | |
| repoPath | |
| ); | |
| if (setUrlResult.exitCode !== 0) { | |
| logger.error('Failed to set origin URL', LOG_CONTEXT, { stderr: setUrlResult.stderr }); | |
| return { isFork: false, error: `Failed to reconfigure remotes: ${setUrlResult.stderr}` }; | |
| } | |
| } else { | |
| const addResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'add', 'origin', forkCloneUrl], | |
| repoPath | |
| ); | |
| if (addResult.exitCode !== 0) { | |
| logger.error('Failed to add origin remote', LOG_CONTEXT, { stderr: addResult.stderr }); | |
| return { isFork: false, error: `Failed to add origin remote: ${addResult.stderr}` }; | |
| } | |
| } | |
| // Set HEAD for origin so getDefaultBranch() works correctly with fork remotes | |
| await execFileNoThrow('git', ['remote', 'set-head', 'origin', '-a'], repoPath); | |
| const renameResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'rename', 'origin', 'upstream'], | |
| repoPath, | |
| env | |
| ); | |
| if (renameResult.exitCode !== 0) { | |
| // Fallback: upstream already exists, just set origin URL | |
| logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, { | |
| stderr: renameResult.stderr, | |
| }); | |
| const setUrlResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'set-url', 'origin', forkCloneUrl], | |
| repoPath, | |
| env | |
| ); | |
| if (setUrlResult.exitCode !== 0) { | |
| logger.error('Failed to set origin URL', LOG_CONTEXT, { stderr: setUrlResult.stderr }); | |
| return { isFork: false, error: `Failed to reconfigure remotes: ${setUrlResult.stderr}` }; | |
| } | |
| } else { | |
| const addResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'add', 'origin', forkCloneUrl], | |
| repoPath, | |
| env | |
| ); | |
| if (addResult.exitCode !== 0) { | |
| logger.error('Failed to add origin remote', LOG_CONTEXT, { stderr: addResult.stderr }); | |
| return { isFork: false, error: `Failed to add origin remote: ${addResult.stderr}` }; | |
| } | |
| } | |
| // Set HEAD for origin so getDefaultBranch() works correctly with fork remotes | |
| await execFileNoThrow('git', ['remote', 'set-head', 'origin', '-a'], repoPath, env); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/utils/symphony-fork.ts` around lines 88 - 120, The git commands that
rewrite remotes and set HEAD (the execFileNoThrow invocations for
['remote','rename','origin','upstream'],
['remote','set-url','origin',forkCloneUrl],
['remote','add','origin',forkCloneUrl'] and ['remote','set-head','origin','-a'])
are called without the expanded environment; update each execFileNoThrow call in
symphony-fork.ts to include the expanded env from getExpandedEnv() (pass it into
the execFileNoThrow options/env parameter) so those git child processes use the
same environment as the earlier gh calls and avoid resolution failures.
Comment on lines
+93
to
+106
| if (renameResult.exitCode !== 0) { | ||
| // Fallback: upstream already exists, just set origin URL | ||
| logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, { | ||
| stderr: renameResult.stderr, | ||
| }); | ||
| const setUrlResult = await execFileNoThrow( | ||
| 'git', | ||
| ['remote', 'set-url', 'origin', forkCloneUrl], | ||
| repoPath | ||
| ); | ||
| if (setUrlResult.exitCode !== 0) { | ||
| logger.error('Failed to set origin URL', LOG_CONTEXT, { stderr: setUrlResult.stderr }); | ||
| return { isFork: false, error: `Failed to reconfigure remotes: ${setUrlResult.stderr}` }; | ||
| } |
There was a problem hiding this comment.
Only use the set-url fallback for the expected upstream already exists case.
Any git remote rename origin upstream failure currently takes the fallback. If the rename failed for a different reason, this can still rewrite origin to the fork and return success without ever establishing a trustworthy upstream remote.
Possible fix
if (renameResult.exitCode !== 0) {
- // Fallback: upstream already exists, just set origin URL
- logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, {
- stderr: renameResult.stderr,
- });
+ const canReuseExistingUpstream = /already exists/i.test(renameResult.stderr);
+ if (!canReuseExistingUpstream) {
+ logger.error('Failed to rename origin to upstream', LOG_CONTEXT, {
+ stderr: renameResult.stderr,
+ });
+ return { isFork: false, error: `Failed to reconfigure remotes: ${renameResult.stderr}` };
+ }
+ logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, {
+ stderr: renameResult.stderr,
+ });
const setUrlResult = await execFileNoThrow(
'git',
['remote', 'set-url', 'origin', forkCloneUrl],
repoPath
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (renameResult.exitCode !== 0) { | |
| // Fallback: upstream already exists, just set origin URL | |
| logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, { | |
| stderr: renameResult.stderr, | |
| }); | |
| const setUrlResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'set-url', 'origin', forkCloneUrl], | |
| repoPath | |
| ); | |
| if (setUrlResult.exitCode !== 0) { | |
| logger.error('Failed to set origin URL', LOG_CONTEXT, { stderr: setUrlResult.stderr }); | |
| return { isFork: false, error: `Failed to reconfigure remotes: ${setUrlResult.stderr}` }; | |
| } | |
| if (renameResult.exitCode !== 0) { | |
| const canReuseExistingUpstream = /already exists/i.test(renameResult.stderr); | |
| if (!canReuseExistingUpstream) { | |
| logger.error('Failed to rename origin to upstream', LOG_CONTEXT, { | |
| stderr: renameResult.stderr, | |
| }); | |
| return { isFork: false, error: `Failed to reconfigure remotes: ${renameResult.stderr}` }; | |
| } | |
| logger.warn('Could not rename origin to upstream, trying set-url fallback', LOG_CONTEXT, { | |
| stderr: renameResult.stderr, | |
| }); | |
| const setUrlResult = await execFileNoThrow( | |
| 'git', | |
| ['remote', 'set-url', 'origin', forkCloneUrl], | |
| repoPath | |
| ); | |
| if (setUrlResult.exitCode !== 0) { | |
| logger.error('Failed to set origin URL', LOG_CONTEXT, { stderr: setUrlResult.stderr }); | |
| return { isFork: false, error: `Failed to reconfigure remotes: ${setUrlResult.stderr}` }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/utils/symphony-fork.ts` around lines 93 - 106, The rename fallback
currently runs for any failure of the git remote rename operation; update the
logic around renameResult (the result of the git remote rename origin upstream
executed via execFileNoThrow) to only attempt the set-url fallback when stderr
or exitCode indicates the specific "remote upstream already exists" error (or an
equivalent recognizable message), otherwise surface/log the original error and
return failure; keep using logger.warn/error, still reference forkCloneUrl and
repoPath for the set-url call when the specific condition matches, and do not
rewrite origin for unrelated rename failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds comprehensive test coverage for fork-related workflows in Symphony IPC handlers and services. The main focus is on ensuring correct handling and persistence of fork information when creating branches and draft pull requests, including error scenarios and cross-fork PRs. The changes introduce mocking for the fork setup utility and validate that fork metadata is correctly passed through the system and used in GitHub CLI commands.
Fork workflow test coverage:
ensureForkSetupfunction fromsymphony-forkin bothsrc/__tests__/main/ipc/handlers/symphony.test.tsandsrc/__tests__/main/services/symphony-runner.test.ts, enabling simulation of fork scenarios in tests. [1] [2]symphony.test.tsfor branch creation and draft PR flows, verifying:ensureForkSetupis called after branch creation and errors are handled if fork setup fails. [1] [2]isFork,forkSlug,upstreamSlug) is persisted in state/metadata and passed togh pr createfor cross-fork PRs, ensuring correct CLI arguments. [1] [2] [3] [4]Test dependency setup:
ensureForkSetupin relevant files, and set default mock return values to simulate both fork and non-fork scenarios. [1] [2] [3]These changes ensure that fork handling logic is robustly tested and that fork metadata is correctly integrated throughout Symphony's contribution and PR creation flows.
closes #531
Summary by CodeRabbit
New Features
Tests