Make pull request creation durable and asynchronous - #715
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes pull request creation for a run durable and asynchronous by shifting POST /api/v1/runs/{id}/pull_request to a 202 Accepted “request accepted” model, adding a status resource (/pull_request/creation), and introducing server-side background processing that can recover pending work after restarts.
Changes:
- Add
PullRequestCreationdomain/API types, project them from run events, and exposeGET /api/v1/runs/{id}/pull_request/creation. - Update server handler to durably append a
pull_request.creation_requestedevent and process requests in a bounded supervisor. - Update clients (Rust + TS) and CLI tests to use the new async contract and poll for completion; add GitHub reconciliation by base/head/commit.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/packages/fabro-api-client/src/models/run-projection.ts | Adds pull_request_creation to the run projection model. |
| lib/packages/fabro-api-client/src/models/pull-request-creation.ts | New OpenAPI-generated model for pull request creation status. |
| lib/packages/fabro-api-client/src/models/pull-request-creation-status.ts | New OpenAPI-generated status union (pending/succeeded/failed). |
| lib/packages/fabro-api-client/src/models/index.ts | Re-exports new pull request creation models. |
| lib/packages/fabro-api-client/src/api/runs-api.ts | Updates create PR return type + adds getRunPullRequestCreation. |
| lib/packages/fabro-api-client/src/.openapi-generator/FILES | Tracks new generated model files. |
| lib/foundation/fabro-types/src/run_projection.rs | Adds pull_request_creation to Rust run projection. |
| lib/foundation/fabro-types/src/run_event/mod.rs | Adds pull_request.creation_requested event body variant. |
| lib/foundation/fabro-types/src/run_event/misc.rs | Defines PullRequestCreationRequestedProps. |
| lib/foundation/fabro-types/src/pull_request.rs | Introduces PullRequestCreation* types and status enum. |
| lib/foundation/fabro-types/src/lib.rs | Re-exports new pull request creation types. |
| lib/foundation/fabro-client/src/client.rs | Polls pull request creation status to keep CLI behavior compatible. |
| lib/foundation/fabro-api/tests/pull_request_round_trip.rs | Ensures API-generated types reuse domain types for PR creation. |
| lib/foundation/fabro-api/src/lib.rs | Re-exports new types through fabro_api::types. |
| lib/foundation/fabro-api/build.rs | Adds OpenAPI→domain type replacements for PR creation types. |
| lib/components/fabro-workflow/src/pipeline/pull_request.rs | Adds GitHub PR reconciliation + shared auto-merge enabling helper. |
| lib/components/fabro-workflow/src/event/names.rs | Maps workflow event enum to the new event name. |
| lib/components/fabro-workflow/src/event/events.rs | Adds workflow event variant for creation requested + logging. |
| lib/components/fabro-workflow/src/event/convert.rs | Converts workflow event to fabro_types::EventBody. |
| lib/components/fabro-store/src/run_state.rs | Projects creation lifecycle into run projection reducer. |
| lib/components/fabro-github/src/lib.rs | Adds GitHub “find existing open PR by base/head/sha” helper + tests. |
| lib/apps/fabro-server/src/server/tests.rs | Updates PR creation tests for 202 + status polling and supervisor recovery. |
| lib/apps/fabro-server/src/server/handler/pull_requests.rs | Implements async PR creation API, status endpoint, and supervisor/worker loop. |
| lib/apps/fabro-server/src/server/handler/mod.rs | Exposes pull_requests handler module for supervisor spawn. |
| lib/apps/fabro-server/src/server.rs | Adds pull-request scheduler notify plumbing and exports supervisor spawner. |
| lib/apps/fabro-server/src/serve.rs | Starts and gracefully shuts down the PR creation supervisor. |
| lib/apps/fabro-server/src/error.rs | Exposes ApiError::detail() for durable failure recording. |
| lib/apps/fabro-cli/tests/it/cmd/pr_create.rs | Updates CLI integration tests to expect async+polling behavior. |
| docs/public/api-reference/fabro-api.yaml | Updates OpenAPI contract for async PR creation + new status endpoint/schema. |
| docs/internal/events.md | Documents the new pull_request.creation_requested event. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Structural cleanup of the durable pull request creation feature, from a three-agent review (reuse, quality, efficiency) of the branch: - Move the supervisor out of handler/ into server/pull_request_supervisor.rs, collapse its double bookkeeping into one task-id map, and fold the five copy-pasted failure arms into attempt_pull_request_creation. - Tag pull_request.failed events with the creation id they resolve, so a publish-stage failure can never fail an unrelated explicit creation. The reducer gains PullRequestCreation::succeed/fail transition methods. - Scan pending creations through a narrow projection-cache accessor instead of materializing every run summary, raise the scan interval to 30s (notify covers the live path), and cap retries for runs whose worker cannot even record a failure. - Answer "creation already pending" POSTs before taking the per-run create lock, which a worker can hold for the whole creation. - Replace the hand-rolled per-run lock map with fabro_store::KeyedMutex. - Reuse cheap Arc'd projections (cached_run_projection) on the poll endpoint and in the worker instead of deep-cloning run summaries and diffs. - Merge ExistingPullRequest into fabro_github::CreatedPullRequest and extract one reconcile_existing_pull_request helper for both call sites. - Give the client poll loop a 15-minute deadline; document that Retry-After and the poll interval are the same constant. - Resolve a wedged pending creation (run already has a pull request) as a durable failure instead of skipping it forever. - Tests: shared wait_for_pull_request_creation helper, a pinned generation- failure assertion, and a new pipeline test proving reconciliation adopts an existing PR without an LLM call or create request. Verified: cargo build --workspace, cargo nextest run --workspace (7,767 passed), nightly clippy -D warnings, fmt --check, insta (no pending), bun typecheck in fabro-api-client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Ran a simplification pass over this branch (three parallel reviews: reuse, quality, efficiency) and pushed the result as one commit. Net −249 lines of Rust outside tests. Highlights: Structure
Correctness
Efficiency
Tests: new pipeline test proving reconciliation adopts an existing PR with zero LLM calls and zero create requests; generation-failure test now pins the error to the LLM step; shared poll helper. Deliberately left for follow-ups: minting the installation token once per Verified: |
# Conflicts: # lib/foundation/fabro-api/src/lib.rs # lib/foundation/fabro-client/src/client.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/apps/fabro-server/src/server/handler/pull_requests.rs:341
create_run_pull_requestvalidates GitHub credentials up front, but it doesn’t validate thatserver_github_contextcan be constructed (e.g.,state.http_client()failure). In that case the endpoint can return 202 Accepted even though GitHub integration is unavailable, and the creation will only fail later in the supervisor. If the intent is to keep the documented 503 behavior for “integration unavailable”, validateserver_github_contexthere too before appending the durable request.
if let Err(err) = load_server_github_credentials(state.as_ref()).await {
return err.into_response();
}
lib/apps/fabro-server/src/server/pull_request_supervisor.rs:173
- The supervisor loop checks
shutdown.is_cancelled()only after the scan/spawn block. If shutdown is triggered whilescan_requestedis true, the supervisor can still enqueue new workers during shutdown (they’ll quickly exit, but still do extra store/cache work and can contend on per-run locks). Check shutdown at the top of the loop before scanning/spawning to avoid scheduling new work during shutdown.
loop {
if scan_requested {
Summary
Why
The old endpoint kept one HTTP request open while it generated the PR description and called GitHub. The client applies a 30-second control-request timeout, so a healthy high-quality model call could finish after the client had already reported a timeout.
The new endpoint persists the request before returning. The long-running work belongs to the server, has a 10-minute server-owned deadline, survives client disconnects and server restarts, and records a durable success or failure result.
Model behavior
Model selection is unchanged. An explicit --model value is preserved. Without an override, the job uses the catalog default for the configured providers. There is no small-model fallback.
Verification