From dd9d6b0c2122aeb113c554e2ad3d0bf38375baa0 Mon Sep 17 00:00:00 2001 From: manjari Date: Wed, 30 Sep 2026 18:16:29 +0000 Subject: [PATCH 1/3] docs(rfc): Add RFC for Phab support in Runway --- doc/rfc/runway/phabricator-support.md | 82 +++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 doc/rfc/runway/phabricator-support.md diff --git a/doc/rfc/runway/phabricator-support.md b/doc/rfc/runway/phabricator-support.md new file mode 100644 index 000000000..bc1afe4ce --- /dev/null +++ b/doc/rfc/runway/phabricator-support.md @@ -0,0 +1,82 @@ +# Phabricator Support in Runway + +## Summary + +Runway rejects `phab://` change URIs. This RFC adds support so Phabricator-based repositories can use SubmitQueue end-to-end. The change applies to both Runway operations (merge-conflict-check and merge) since they share one code path. + +## Background + +The `phab://` URI scheme, the `ChangeProvider` (Conduit), and the routing provider already work. The gap is in Runway's git merger — the only component that resolves a change URI into a git commit. + +A GitHub URI embeds the commit SHA: `github://{host}/{org}/{repo}/pull/{pr}/{head_sha}`. A Phabricator URI does not: `phab://{host}/D{revision}/{diff_id}`. The DiffID pins the code state, but the commit only exists at a staging ref (`refs/tags/phabricator/diff/{diff_id}`) that Phabricator pushes when staging areas are enabled. The SHA is unknown until fetched. + +## Proposal + +### 1. `phab://` case in `resolveChange` + +`resolveChange` becomes a method on `gitMerger` so the phab case can read the configured staging ref format: + +```go +case "phab": + cid, _ := entityphab.ParseChangeID(uri) + return changeRef{ + Provider: scheme, + Ref: fmt.Sprintf(m.phabStagingRefFormat, cid.DiffID), + Label: cid.Revision(), // "D12345" + }, nil +``` + +### 2. SHA resolution in `ensureObject` + +When SHA is empty, fetch the staging ref and resolve the commit. Both empty is a terminal error. + +```go +// ensureObject — new dispatch at the top: +if ref.SHA == "" && ref.Ref == "" { return error: no SHA and no ref } +if ref.SHA == "" { return resolveObjectFromRef(ref) } +// ... existing by-SHA flow unchanged ... + +// resolveObjectFromRef: +git fetch +ref.SHA = git rev-parse FETCH_HEAD +return ref +``` + +### 3. Value-returning signatures + +`changeRef` is a value type. The merger copies refs into a flattened slice for `ensureObjects`/`checkStale`, but `applySteps` reads the originals — a SHA resolved on the copy never reaches the apply paths. + +```go +// Before — ensureObjects returns nothing, resolved SHAs lost: +ensureObjects(refs []changeRef) error + +// After — resolution returns values, caller reassigns: +ensureObject(ref changeRef) (changeRef, error) +ensureObjects(refs []changeRef) ([]changeRef, error) +ensureStepObjects(steps []resolvedStep) ([]resolvedStep, error) // new + +// applyTransforming: +steps, _ = m.ensureStepObjects(ctx, steps) // reassign with resolved SHAs +m.checkStale(ctx, stepChangeRefs(steps)) // sees resolved SHAs +// tryApply uses the resolved steps +``` + +### 4. Configurable staging ref format + +`Params.PhabStagingRefFormat` defaults to `refs/tags/phabricator/diff/%d`. Validated at startup to contain a `%d` placeholder. + +```yaml +defaults: + merger: + type: git + phabStagingRefFormat: "refs/tags/phabricator/diff/%d" # default, omit if stock +``` + +## Scope, limitations, and prerequisites + +**Prerequisites:** +- **Staging areas must be enabled** on the Phabricator instance. Without staging, the ref doesn't exist and the fetch fails terminally (`ErrInvalidRequest`). The staging ref format is configurable via `phabStagingRefFormat` in the YAML config for non-standard layouts. + +**Limitations:** +- **No merge-time staleness detection.** For GitHub, `checkStale` catches force-pushes between validate and merge via `ls-remote`. For Phab, each `arc diff` creates a new diff ID with its own immutable staging ref, so this check always passes. Staleness detection must happen upstream at the validate stage via Conduit. If the validate-to-merge window proves problematic, the long-term fix is a pluggable staleness check. +- **Merge strategy.** Phabricator workflows produce linear git history (one commit per change, no merge commits). Queues serving Phab repos should use SQUASH_REBASE or REBASE to match this convention. The MERGE strategy works but produces non-linear history which is atypical for Phab history. From 44b6b37133858ef1015686f72a2c9cdaa258e2a2 Mon Sep 17 00:00:00 2001 From: manjari Date: Wed, 30 Sep 2026 22:03:19 +0000 Subject: [PATCH 2/3] docs(rfc): Updade phab support RFC in Runway to include object resolver --- doc/rfc/runway/phabricator-support.md | 85 +++++++++++++++++---------- 1 file changed, 54 insertions(+), 31 deletions(-) diff --git a/doc/rfc/runway/phabricator-support.md b/doc/rfc/runway/phabricator-support.md index bc1afe4ce..28bb982c2 100644 --- a/doc/rfc/runway/phabricator-support.md +++ b/doc/rfc/runway/phabricator-support.md @@ -12,9 +12,53 @@ A GitHub URI embeds the commit SHA: `github://{host}/{org}/{repo}/pull/{pr}/{hea ## Proposal -### 1. `phab://` case in `resolveChange` +### 1. `ObjectResolver` interface -`resolveChange` becomes a method on `gitMerger` so the phab case can read the configured staging ref format: +Object resolution (fetching commits, resolving refs to SHAs, checking staleness) is currently embedded in the git merger. This RFC extracts it into a pluggable interface so the same orchestration logic works with any VCS backend — the OSS git implementation, or an alternative implementation that a deployment substitutes. + +``` +runway/extension/merger/ +├── merger.go — Merger interface (Merge, CheckMergeability) +├── object/ +│ └── resolver.go — ObjectResolver interface + shared orchestration +├── git/ +│ ├── git_merger.go — Merger impl +│ ├── changeref.go — resolveChange (URI → changeRef) +│ └── objects.go — gitResolver (implements ObjectResolver via git CLI) +├── fake/ +├── noop/ +└── mock/ +``` + +The `ObjectResolver` interface: + +```go +// package object + +type ObjectResolver interface { + HasCommit(ctx context.Context, sha string) bool + FetchBySHA(ctx context.Context, sha string) error + FetchByRef(ctx context.Context, ref string) error + ResolveHead(ctx context.Context) (string, error) + IsRemoteReachable(ctx context.Context) error + LSRemote(ctx context.Context, ref string) (string, error) +} +``` + +Shared orchestration lives in the same package and operates on `ObjectResolver`: + +```go +func EnsureObject(ctx context.Context, r ObjectResolver, ref changeRef) (changeRef, error) +func EnsureObjects(ctx context.Context, r ObjectResolver, refs []changeRef) ([]changeRef, error) +func EnsureStepObjects(ctx context.Context, r ObjectResolver, steps []resolvedStep) ([]resolvedStep, error) +func CheckStale(ctx context.Context, r ObjectResolver, refs []changeRef) error +``` + +The OSS git merger provides a `gitResolver` in `merger/git/` that implements `ObjectResolver` by shelling out to the git CLI. Alternative deployments provide their own implementation. Both plug into the same shared orchestration. + +### 2. `phab://` case in `resolveChange` + +Each `Merger` implementation has its own URI-to-change resolution. The OSS git merger adds a `phab` case to `resolveChange`: ```go case "phab": @@ -26,40 +70,19 @@ case "phab": }, nil ``` -### 2. SHA resolution in `ensureObject` +Alternative deployments need an equivalent change in their own resolution logic. The parsing is ~15 lines and uses the shared `entityphab.ParseChangeID` from `platform/base/change/phabricator/`. If consolidation is desired later, `resolveChange` and `changeRef` can be moved to the `object/` package as exported shared code. + +### 3. SHA resolution in `EnsureObject` -When SHA is empty, fetch the staging ref and resolve the commit. Both empty is a terminal error. +When SHA is empty, fetch the staging ref and resolve the commit. Both empty is a terminal error: ```go -// ensureObject — new dispatch at the top: if ref.SHA == "" && ref.Ref == "" { return error: no SHA and no ref } -if ref.SHA == "" { return resolveObjectFromRef(ref) } +if ref.SHA == "" { fetch ref.Ref → resolve SHA via ResolveHead → return ref } // ... existing by-SHA flow unchanged ... - -// resolveObjectFromRef: -git fetch -ref.SHA = git rev-parse FETCH_HEAD -return ref ``` -### 3. Value-returning signatures - -`changeRef` is a value type. The merger copies refs into a flattened slice for `ensureObjects`/`checkStale`, but `applySteps` reads the originals — a SHA resolved on the copy never reaches the apply paths. - -```go -// Before — ensureObjects returns nothing, resolved SHAs lost: -ensureObjects(refs []changeRef) error - -// After — resolution returns values, caller reassigns: -ensureObject(ref changeRef) (changeRef, error) -ensureObjects(refs []changeRef) ([]changeRef, error) -ensureStepObjects(steps []resolvedStep) ([]resolvedStep, error) // new - -// applyTransforming: -steps, _ = m.ensureStepObjects(ctx, steps) // reassign with resolved SHAs -m.checkStale(ctx, stepChangeRefs(steps)) // sees resolved SHAs -// tryApply uses the resolved steps -``` +`EnsureObject` returns the `changeRef` with SHA set. `EnsureStepObjects` returns updated steps so the resolved SHAs propagate to all downstream paths (`checkStale`, `applySteps`, `promote`). `changeRef` stays a value type — resolution returns new values rather than mutating shared state. ### 4. Configurable staging ref format @@ -78,5 +101,5 @@ defaults: - **Staging areas must be enabled** on the Phabricator instance. Without staging, the ref doesn't exist and the fetch fails terminally (`ErrInvalidRequest`). The staging ref format is configurable via `phabStagingRefFormat` in the YAML config for non-standard layouts. **Limitations:** -- **No merge-time staleness detection.** For GitHub, `checkStale` catches force-pushes between validate and merge via `ls-remote`. For Phab, each `arc diff` creates a new diff ID with its own immutable staging ref, so this check always passes. Staleness detection must happen upstream at the validate stage via Conduit. If the validate-to-merge window proves problematic, the long-term fix is a pluggable staleness check. -- **Merge strategy.** Phabricator workflows produce linear git history (one commit per change, no merge commits). Queues serving Phab repos should use SQUASH_REBASE or REBASE to match this convention. The MERGE strategy works but produces non-linear history which is atypical for Phab history. +- **No merge-time staleness detection.** For GitHub, `CheckStale` catches force-pushes between validate and merge via `LSRemote`. For Phab, each `arc diff` creates a new diff ID with its own immutable staging ref, so this check always passes. Staleness detection must happen upstream at the validate stage via Conduit. If the validate-to-merge window proves problematic, the long-term fix is a pluggable staleness check. +- **Merge strategy.** Phabricator workflows produce linear git history (one commit per change, no merge commits). Queues serving Phab repos should use SQUASH_REBASE or REBASE to match this convention. The MERGE strategy works but produces non-linear history which is atypical for Phab history. \ No newline at end of file From b93a5f5c993e774e673b4910d95178a7185f589e Mon Sep 17 00:00:00 2001 From: manjari Date: Thu, 1 Oct 2026 22:11:54 +0000 Subject: [PATCH 3/3] docs(rfc): Updade phab support RFC in Runway to consolidate merger logic --- doc/rfc/runway/phabricator-support.md | 103 +++++++++++++------------- 1 file changed, 52 insertions(+), 51 deletions(-) diff --git a/doc/rfc/runway/phabricator-support.md b/doc/rfc/runway/phabricator-support.md index 28bb982c2..02742eaaa 100644 --- a/doc/rfc/runway/phabricator-support.md +++ b/doc/rfc/runway/phabricator-support.md @@ -6,59 +6,69 @@ Runway rejects `phab://` change URIs. This RFC adds support so Phabricator-based ## Background -The `phab://` URI scheme, the `ChangeProvider` (Conduit), and the routing provider already work. The gap is in Runway's git merger — the only component that resolves a change URI into a git commit. +The `phab://` URI scheme, the `ChangeProvider` (Conduit), and the routing provider already work. The gap is in Runway's merger — the only component that resolves a change URI into a git commit. A GitHub URI embeds the commit SHA: `github://{host}/{org}/{repo}/pull/{pr}/{head_sha}`. A Phabricator URI does not: `phab://{host}/D{revision}/{diff_id}`. The DiffID pins the code state, but the commit only exists at a staging ref (`refs/tags/phabricator/diff/{diff_id}`) that Phabricator pushes when staging areas are enabled. The SHA is unknown until fetched. ## Proposal -### 1. `ObjectResolver` interface +### 1. `gitworkspace.Workspace` extension -Object resolution (fetching commits, resolving refs to SHAs, checking staleness) is currently embedded in the git merger. This RFC extracts it into a pluggable interface so the same orchestration logic works with any VCS backend — the OSS git implementation, or an alternative implementation that a deployment substitutes. +Git operations (fetch, merge, push, rev-parse) are currently embedded in each merger implementation. This RFC extracts them into a shared `platform/extension/gitworkspace/` contract so the same merger logic works with any git backend. +```go +// platform/extension/gitworkspace/ + +type Workspace interface { + Exec(commands []Command) ([]Output, error) + Close() error +} + +type Factory interface { + For(ctx context.Context, repo string) (Workspace, error) +} ``` -runway/extension/merger/ -├── merger.go — Merger interface (Merge, CheckMergeability) -├── object/ -│ └── resolver.go — ObjectResolver interface + shared orchestration -├── git/ -│ ├── git_merger.go — Merger impl -│ ├── changeref.go — resolveChange (URI → changeRef) -│ └── objects.go — gitResolver (implements ObjectResolver via git CLI) -├── fake/ -├── noop/ -└── mock/ -``` -The `ObjectResolver` interface: +`Command` carries an `Alias`, `Bin`, `Args`, and optional `Stdin`. `Output` carries the matching `Alias`, `ExitCode`, `Stdout`, and `Stderr`. Commands within one `Exec` call run in order; a non-zero exit code skips the remaining commands in that batch. State persists across `Exec` calls on the same `Workspace`. + +Two implementations: + +- **OSS** (`platform/extension/gitworkspace/local/`): executes commands as local subprocesses against a git clone, with skip-on-failure semantics. +- **Alternative deployments**: wrap their backend's exec API in the same contract — a thin type-translation adapter. + +### 2. `merger.CommitMessageResolver` + +Squash and merge commits need a commit message and authorship. The source differs by deployment: the OSS merger builds a synthetic message from the step ID and change label and reads authorship from the git object; other deployments may resolve richer metadata from their code review platform. ```go -// package object - -type ObjectResolver interface { - HasCommit(ctx context.Context, sha string) bool - FetchBySHA(ctx context.Context, sha string) error - FetchByRef(ctx context.Context, ref string) error - ResolveHead(ctx context.Context) (string, error) - IsRemoteReachable(ctx context.Context) error - LSRemote(ctx context.Context, ref string) (string, error) +// runway/extension/merger/ + +type CommitMessageResolver interface { + Resolve(ctx context.Context, uri string) (CommitMessage, error) +} + +type CommitMessage struct { + Message string + AuthorName string + AuthorEmail string } ``` -Shared orchestration lives in the same package and operates on `ObjectResolver`: +The merger takes a `CommitMessageResolver` as a constructor dependency alongside the `gitworkspace.Factory`. + +### 3. Unified merger + +With `Workspace` and `CommitMessageResolver` injected, the merger becomes a single implementation that builds command batches, sends them to the workspace, and evaluates results. The current OSS git merger and any alternative deployment merger collapse into one codebase. The merger constructor takes both dependencies: ```go -func EnsureObject(ctx context.Context, r ObjectResolver, ref changeRef) (changeRef, error) -func EnsureObjects(ctx context.Context, r ObjectResolver, refs []changeRef) ([]changeRef, error) -func EnsureStepObjects(ctx context.Context, r ObjectResolver, steps []resolvedStep) ([]resolvedStep, error) -func CheckStale(ctx context.Context, r ObjectResolver, refs []changeRef) error +func NewMerger(ws gitworkspace.Factory, cmr merger.CommitMessageResolver, ...) merger.Merger ``` -The OSS git merger provides a `gitResolver` in `merger/git/` that implements `ObjectResolver` by shelling out to the git CLI. Alternative deployments provide their own implementation. Both plug into the same shared orchestration. +Strategy dispatch (SQUASH_REBASE, REBASE, MERGE, PROMOTE), conflict detection, staleness checking, and result building all live in this single merger — no per-backend duplication. -### 2. `phab://` case in `resolveChange` +### 4. `phab://` case in `resolveChange` -Each `Merger` implementation has its own URI-to-change resolution. The OSS git merger adds a `phab` case to `resolveChange`: +The unified merger's URI-to-change resolution adds a `phab` case: ```go case "phab": @@ -70,36 +80,27 @@ case "phab": }, nil ``` -Alternative deployments need an equivalent change in their own resolution logic. The parsing is ~15 lines and uses the shared `entityphab.ParseChangeID` from `platform/base/change/phabricator/`. If consolidation is desired later, `resolveChange` and `changeRef` can be moved to the `object/` package as exported shared code. - -### 3. SHA resolution in `EnsureObject` +### 5. SHA resolution in `ensureObject` When SHA is empty, fetch the staging ref and resolve the commit. Both empty is a terminal error: ```go if ref.SHA == "" && ref.Ref == "" { return error: no SHA and no ref } -if ref.SHA == "" { fetch ref.Ref → resolve SHA via ResolveHead → return ref } -// ... existing by-SHA flow unchanged ... +if ref.SHA == "" { fetch ref.Ref → resolve SHA → return ref } +// existing by-SHA flow unchanged ``` -`EnsureObject` returns the `changeRef` with SHA set. `EnsureStepObjects` returns updated steps so the resolved SHAs propagate to all downstream paths (`checkStale`, `applySteps`, `promote`). `changeRef` stays a value type — resolution returns new values rather than mutating shared state. +`ensureObject` returns the `changeRef` with SHA set. `changeRef` stays a value type — resolution returns new values rather than mutating shared state. -### 4. Configurable staging ref format +### 6. Configurable staging ref format -`Params.PhabStagingRefFormat` defaults to `refs/tags/phabricator/diff/%d`. Validated at startup to contain a `%d` placeholder. - -```yaml -defaults: - merger: - type: git - phabStagingRefFormat: "refs/tags/phabricator/diff/%d" # default, omit if stock -``` +`PhabStagingRefFormat` defaults to `refs/tags/phabricator/diff/%d`. Validated at startup to contain a `%d` placeholder. Configurable via YAML for non-standard Phabricator layouts. ## Scope, limitations, and prerequisites **Prerequisites:** -- **Staging areas must be enabled** on the Phabricator instance. Without staging, the ref doesn't exist and the fetch fails terminally (`ErrInvalidRequest`). The staging ref format is configurable via `phabStagingRefFormat` in the YAML config for non-standard layouts. +- **Staging areas must be enabled** on the Phabricator instance. Without staging, the ref doesn't exist and the fetch fails terminally (`ErrInvalidRequest`). The staging ref format is configurable via `phabStagingRefFormat` for non-standard layouts. **Limitations:** -- **No merge-time staleness detection.** For GitHub, `CheckStale` catches force-pushes between validate and merge via `LSRemote`. For Phab, each `arc diff` creates a new diff ID with its own immutable staging ref, so this check always passes. Staleness detection must happen upstream at the validate stage via Conduit. If the validate-to-merge window proves problematic, the long-term fix is a pluggable staleness check. -- **Merge strategy.** Phabricator workflows produce linear git history (one commit per change, no merge commits). Queues serving Phab repos should use SQUASH_REBASE or REBASE to match this convention. The MERGE strategy works but produces non-linear history which is atypical for Phab history. \ No newline at end of file +- **No merge-time staleness detection.** For GitHub, `checkStale` catches force-pushes between validate and merge via `ls-remote`. For Phab, each `arc diff` creates a new diff ID with its own immutable staging ref, so this check always passes. Staleness detection must happen upstream at the validate stage via Conduit. If the validate-to-merge window proves problematic, the long-term fix is a pluggable staleness check. +- **Merge strategy.** Phabricator workflows produce linear git history (one commit per change, no merge commits). Queues serving Phab repos should use SQUASH_REBASE or REBASE to match this convention. The MERGE strategy works but produces non-linear history which is atypical for Phab history.