feat: add driver abstraction for PR stacking tool support#60
Open
nvandessel wants to merge 8 commits intomainfrom
Open
feat: add driver abstraction for PR stacking tool support#60nvandessel wants to merge 8 commits intomainfrom
nvandessel wants to merge 8 commits intomainfrom
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Owner
Author
|
converting to draft until I test with graphite. I think this is a good foundation though. |
Owner
Author
|
@greptile review |
Greptile SummaryThis PR introduces a clean Key observations:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| internal/driver/driver.go | Defines the Driver interface, PushOpts/PushResult types, RebaseConflictError, PR state constants, and the Resolve factory. Interface is clean and well-documented; PR state constants and error types are properly exported. |
| internal/driver/graphite.go | Embeds Native and overrides CreateBranch/Push/Rebase for gt CLI. The PR number is correctly parsed from gt submit output via submitLineRe and parseSubmitResult. However, when ExistingPR is non-nil the gt submit output is always discarded (minor clarity issue), and the Rebase implementation hardcodes Branch:"stack" in RebaseConflictError (harmless since sync.go uses its own loop variable). |
| internal/driver/mock.go | Stateful in-memory mock with override hooks for all driver methods. Well-structured; correctly models branch creation, checkout, and PR state. StackComments flag correctly controls SupportsStackComments() for driver-specific test scenarios. |
| cmd/push.go | Correctly delegates push/PR operations to the driver. Critical bug: PR number is only saved to state when result.Created==true. With the Graphite driver, gt submit can output "(updated)" for a branch whose PR was created implicitly by a prior stack submission, causing the PR number to be permanently lost from frond's state. |
| cmd/helpers.go | Adds driverOverride global for test injection and resolveDriver() helper. Works correctly for current sequential tests but driverOverride is unprotected against concurrent access — a latent race if any future tests use t.Parallel(). |
| cmd/init.go | New frond init command validates driver availability, acquires lock, and persists driver name to state. Correctly normalises "native" to "" for storage and uses drv.Name() (returns "native") in output. State preservation across re-init is verified by tests. |
| cmd/sync.go | Correctly refactored to use driver interface for fetch, rebase, PR state queries, and retargeting. Conflict handling via driver.RebaseConflictError works correctly since conflictBranch is set from the loop variable (not conflictErr.Branch), making the "stack" sentinel in Graphite's error irrelevant. |
| cmd/cmd_test.go | Significantly simplified by replacing real git repos and fake-gh PATH manipulation with the mock driver. New tests for init command, sync with merged PRs, rebase conflicts, and driver resolution are thorough. withFakeGH() correctly scoped to only tests that need the gh comment API. |
Sequence Diagram
sequenceDiagram
participant User
participant frond CLI
participant resolveDriver
participant Native Driver
participant Graphite Driver
participant git/gh CLIs
participant gt CLI
User->>frond CLI: frond init --driver graphite
frond CLI->>resolveDriver: Resolve("graphite")
resolveDriver->>gt CLI: exec.LookPath("gt")
gt CLI-->>resolveDriver: found
resolveDriver-->>frond CLI: *Graphite
frond CLI->>frond CLI: state.Write (driver: "graphite")
User->>frond CLI: frond push
frond CLI->>resolveDriver: resolveDriver(state)
alt driver == "" or "native"
resolveDriver-->>frond CLI: *Native
frond CLI->>Native Driver: Push(PushOpts)
Native Driver->>git/gh CLIs: git push + gh pr create/edit
git/gh CLIs-->>Native Driver: PRNumber, Created
Native Driver-->>frond CLI: PushResult
else driver == "graphite"
resolveDriver-->>frond CLI: *Graphite
frond CLI->>Graphite Driver: Push(PushOpts)
Graphite Driver->>gt CLI: gt submit --no-interactive --no-edit
gt CLI-->>Graphite Driver: "branch: https://.../42 (created)"
Graphite Driver->>Graphite Driver: parseSubmitResult(out, branch)
Graphite Driver-->>frond CLI: PushResult{PRNumber:42, Created:true}
end
frond CLI->>frond CLI: state.Write (if Created)
User->>frond CLI: frond sync
frond CLI->>resolveDriver: resolveDriver(state)
frond CLI->>Native Driver: Fetch / PRState / Rebase / RetargetPR
Note over Native Driver,gt CLI: Graphite driver uses gt restack instead of git rebase
Comments Outside Diff (1)
-
cmd/helpers.go, line 30 (link)driverOverridenot mutex-protecteddriverOverrideis a package-level variable mutated by tests (set insetupTestEnv, cleared in cleanup). No current test callst.Parallel(), but if any are parallelised in the future this will introduce a data race. Wrapping with async.Mutex(or usingatomic.Value) would make the override safe to use in parallel tests:var ( driverOverrideMu sync.Mutex driverOverride driver.Driver ) func resolveDriver(st *state.State) (driver.Driver, error) { driverOverrideMu.Lock() ovr := driverOverride driverOverrideMu.Unlock() if ovr != nil { return ovr, nil } return driver.Resolve(st.Driver) }
Last reviewed commit: b4a8e5e
Owner
Author
Code reviewFound 1 issue:
frond/internal/driver/graphite.go Lines 47 to 56 in 97e88c4 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Introduce a Driver interface that abstracts all branch/PR/git operations, enabling frond to delegate to different stacking tools (native git+gh, Graphite, etc.) while managing the DAG layer on top. Key changes: - internal/driver/: Driver interface, Native (git+gh), Graphite (gt), and Mock implementations with PushOpts/PushResult/RebaseConflictError - internal/state: Add Driver field, export GitCommonDir for testability - cmd/: Refactor all commands (new, push, sync, status, track, untrack) to use driver instead of direct git/gh imports - cmd/init.go: New `frond init [--driver]` command - cmd/cmd_test.go: Full rewrite using mock driver (no git/gh subprocess) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix Graphite.Rebase discarding caller's context (used background ctx)
- Remove unnecessary local variable in Native.Push
- Add tests: sync merged-PR path, sync rebase conflict (ExitError code 2),
resolveDriver with state/override, Resolve("graphite") skip-if-not-installed
- cmd coverage: 78.4% -> 86.2%
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace lookupPRByBranch (which shelled out to `gh pr view`) with parsePRNumber that extracts PR numbers directly from `gt submit` output. This removes the unnecessary `gh` CLI dependency from the Graphite driver. Also: tighten Mock.Checkout to validate branch existence, delete unused fakegt test double, add table-driven tests for parsePRNumber. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pass opts.Body as --description to gt submit (was silently dropped) - Return created/updated status from parseSubmitResult instead of hardcoding Created: true (uses the captured group from gt output) - Add PR state constants to driver package, replace magic string "MERGED" in sync.go (reverts regression from PR #20's fix) - Add NewNative() constructor with gh.Available() check, matching the pattern established by NewGraphite() for gt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Build a fakegt test double (env-var-controlled) via TestMain and prepend it to PATH, then test the Graphite driver end-to-end in isolation without needing the real gt CLI or GitHub. Tests cover: NewGraphite, CreateBranch (with real temp git repo), Push (created/updated/existing PR paths), --title/--description/ --draft flag forwarding, Push failure, Rebase success, and Rebase conflict detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Graphite manages its own stack visualization on PRs, so frond should skip posting stack comments when using the Graphite driver. Add SupportsStackComments() to the Driver interface (native=true, graphite=false) and guard updateStackComments/updateMergedComments calls in push and sync commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nvandessel
force-pushed
the
feature/frond-graphite
branch
from
82a59e8 to
da84d19
Compare
Owner
Author
|
@greptileai review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Graphite.CreateBranch was bypassing the driver abstraction by calling git.Checkout directly. Use g.Checkout (inherited from Native) to stay consistent with the driver pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+108
to
114
| if result.Created { | ||
| br.PR = &result.PRNumber | ||
| st.Branches[branch] = br | ||
| if err := state.Write(ctx, st); err != nil { | ||
| return fmt.Errorf("writing state: %w", err) | ||
| } | ||
| created = true | ||
| } else { | ||
| // 8. PR exists — check if base needs retargeting. | ||
| prNumber = *br.PR | ||
|
|
||
| info, err := gh.PRView(ctx, prNumber) | ||
| if err != nil { | ||
| return fmt.Errorf("viewing PR #%d: %w", prNumber, err) | ||
| } | ||
|
|
||
| if info.BaseRefName != br.Parent { | ||
| if err := gh.PREdit(ctx, prNumber, br.Parent); err != nil { | ||
| return fmt.Errorf("retargeting PR #%d: %w", prNumber, err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
PR number silently dropped for Graphite (updated) result
With the Graphite driver, gt submit always submits the entire stack. When a user first pushes branch B (downstream), gt submit creates PRs for both branch A and branch B — but frond only records the PR number for branch B (since that is opts.Branch). Later, when the user runs frond push on branch A, gt submit outputs (updated) for branch A because its PR already exists on GitHub. parseSubmitResult correctly extracts the PR number, but result.Created is false, so this block is skipped and the PR number is never persisted to state.
This leaves branch A's br.PR == nil permanently, so future frond status --fetch and frond sync operations cannot see or retarget the PR.
The condition should also save the number when frond doesn't yet have a record of the PR (br.PR == nil):
Suggested change
| if result.Created { | |
| br.PR = &result.PRNumber | |
| st.Branches[branch] = br | |
| if err := state.Write(ctx, st); err != nil { | |
| return fmt.Errorf("writing state: %w", err) | |
| } | |
| created = true | |
| } else { | |
| // 8. PR exists — check if base needs retargeting. | |
| prNumber = *br.PR | |
| info, err := gh.PRView(ctx, prNumber) | |
| if err != nil { | |
| return fmt.Errorf("viewing PR #%d: %w", prNumber, err) | |
| } | |
| if info.BaseRefName != br.Parent { | |
| if err := gh.PREdit(ctx, prNumber, br.Parent); err != nil { | |
| return fmt.Errorf("retargeting PR #%d: %w", prNumber, err) | |
| } | |
| } | |
| } | |
| if result.Created || br.PR == nil { | |
| br.PR = &result.PRNumber | |
| st.Branches[branch] = br | |
| if err := state.Write(ctx, st); err != nil { | |
| return fmt.Errorf("writing state: %w", err) | |
| } | |
| } |
Comment on lines
+61
to
+63
| // For existing PRs, return the existing number. | ||
| if opts.ExistingPR != nil { | ||
| return &PushResult{PRNumber: *opts.ExistingPR, Created: false}, nil |
There was a problem hiding this comment.
(updated) output status is silently ignored when ExistingPR is set
When opts.ExistingPR != nil, gt submit is still executed and may report that the stack has changed (e.g., a previously-draft PR is now ready), but the result is discarded in favour of the stored PR number. While this is benign today (the PR number is stable), a comment explaining the deliberate choice to skip parsing would help future readers understand why the output is intentionally thrown away here.
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
2 participants
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.
Summary
Driverinterface that abstracts all git/PR operations (branch creation, push, rebase, PR state queries), enabling frond to work with different stacking tools interchangeablyfrond initcommand for configuring driver selection per-repo (stored infrond.json)new,push,sync,status,track,untrack) to use the driver interface instead of directly calling git/ghgt submitoutput directly for PR numbers instead of shelling out togh— no unnecessaryghdependency in the Graphite driverTest plan
go build ./...passesgo test ./...— all tests pass including newparsePRNumbertable-driven testsgo vet ./...— no issuesgolangci-lint run— 0 issuesgofmt -l .— no formatting issuesfrond initcreates/updatesfrond.jsonwith driver fieldfrond pushworks with native driver (existing behavior)frond pushworks with graphite driver (requiresgtinstalled)🤖 Generated with Claude Code