Repository navigation
feat(node): add a commit status reporting surface #304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
04263db
af17831
ac31868
6663fed
7be9f08
db3681e
90b8294
ad71f79
806f902
612d9d2
d3074a6
5445dda
61033af
a4e2b9f
8652b86
6b0763a
e9e5e3d
41bbe70
48e8897
600c358
862293c
9519078
ddc4a2c
e8952f5
20f725e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,7 @@ pub async fn create_pr( | |
| status: "open".to_string(), | ||
| merged_by_did: None, | ||
| merged_at: None, | ||
| head_commit: None, | ||
| created_at: now.clone(), | ||
| updated_at: now, | ||
| }; | ||
|
|
@@ -225,6 +226,17 @@ pub async fn merge_pr( | |
| .map_err(|e| crate::api::repos::acquire_write_app_error(&e, &name))?; | ||
| let disk_path = guard.path().to_path_buf(); | ||
| let merger_did = auth.0; | ||
|
|
||
| // The source head this merge is about to consume, read under the write lock. | ||
| // The PR row above was loaded before the lock, so a push landing in between | ||
| // already moved the branch and the row's own `head_commit` is stale; the | ||
| // value frozen on the merged PR has to be what was merged. `None` when the | ||
| // ref cannot be resolved, which leaves whatever the push path last stored. | ||
| let merged_source_head = | ||
| store::read_ref(&disk_path, &store::local_branch_ref(&pr.source_branch)) | ||
| .ok() | ||
| .flatten(); | ||
|
|
||
| let merge_result = store::merge_branch( | ||
| &disk_path, | ||
| &pr.target_branch, | ||
|
|
@@ -241,7 +253,10 @@ pub async fn merge_pr( | |
|
|
||
| let merge_sha = merge_result.map_err(|e| AppError::Git(e.to_string()))?; | ||
|
|
||
| state.db.merge_pr(&pr.id, &merger_did).await?; | ||
| state | ||
| .db | ||
| .merge_pr(&pr.id, &merger_did, merged_source_head.as_deref()) | ||
| .await?; | ||
| let _ = state.db.touch_repo(&record.id).await; | ||
|
|
||
| webhooks::fire_event( | ||
|
|
@@ -291,7 +306,36 @@ pub async fn close_pr( | |
| )); | ||
| } | ||
|
|
||
| state.db.close_pr(&pr.id).await?; | ||
| // Same refusal merge_pr gives, before the head is resolved: a repeated close | ||
| // must not store the branch's current tip on a historical pull request. | ||
| if pr.status != "open" { | ||
| return Err(AppError::BadRequest(format!("PR is already {}", pr.status))); | ||
| } | ||
|
|
||
| // Freeze the head this close decided against, the same snapshot merge_pr | ||
| // takes on its path: a closed pull request's rollup serves only the stored | ||
| // head, so a PR closed before its first status read would report | ||
| // head_resolved=false forever. The recorded push for the branch is the | ||
| // canonical answer; the disk read covers a push whose event write failed. | ||
| // An unresolvable head keeps whatever the push path last stored. | ||
| let branch_ref = store::local_branch_ref(&pr.source_branch); | ||
| let closed_head = if pr.head_commit.is_some() { | ||
| None | ||
| } else { | ||
| state | ||
| .db | ||
| .latest_push_sha_for_ref(&record.id, &branch_ref) | ||
| .await | ||
| .ok() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Propagate the head-lookup error before closing the pull request. If Propagate the error with Suggested fix
|
||
| .flatten() | ||
| .or_else(|| { | ||
| store::read_ref(std::path::Path::new(&record.disk_path), &branch_ref) | ||
| .ok() | ||
| .flatten() | ||
| }) | ||
| .map(|sha| sha.to_ascii_lowercase()) | ||
| }; | ||
| state.db.close_pr(&pr.id, closed_head.as_deref()).await?; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| webhooks::fire_event( | ||
| std::sync::Arc::clone(&state.db), | ||
|
|
@@ -437,6 +481,162 @@ pub async fn list_comments( | |
| Ok(Json(serde_json::json!({ "comments": comments }))) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use axum::body::Body; | ||
| use axum::http::{Method, StatusCode}; | ||
| use axum::routing::post; | ||
| use axum::Router; | ||
| use std::process::Command; | ||
| use tower::ServiceExt; | ||
|
|
||
| const OWNER_DID: &str = "did:key:z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; | ||
| const STALE_SHA: &str = "9999999999999999999999999999999999999999"; | ||
|
|
||
| fn git(dir: &std::path::Path, args: &[&str]) { | ||
| let out = Command::new("git") | ||
| .args(args) | ||
| .current_dir(dir) | ||
| .env("GIT_AUTHOR_NAME", "t") | ||
| .env("GIT_AUTHOR_EMAIL", "t@example.com") | ||
| .env("GIT_COMMITTER_NAME", "t") | ||
| .env("GIT_COMMITTER_EMAIL", "t@example.com") | ||
| .output() | ||
| .expect("git runs"); | ||
| assert!( | ||
| out.status.success(), | ||
| "git {args:?} failed: {}", | ||
| String::from_utf8_lossy(&out.stderr) | ||
| ); | ||
| } | ||
|
|
||
| /// A bare repo with `main` and a `feature` branch one commit ahead. | ||
| /// Returns the bare path and the real `feature` tip. | ||
| fn bare_repo_with_feature( | ||
| root: &std::path::Path, | ||
| owner_slug: &str, | ||
| ) -> (std::path::PathBuf, String) { | ||
| let work = root.join("work"); | ||
| std::fs::create_dir_all(&work).unwrap(); | ||
| git(&work, &["init", "-q", "-b", "main", "."]); | ||
| std::fs::write(work.join("a.txt"), "one").unwrap(); | ||
| git(&work, &["add", "."]); | ||
| git(&work, &["commit", "-qm", "one"]); | ||
| git(&work, &["checkout", "-qb", "feature"]); | ||
| std::fs::write(work.join("b.txt"), "two").unwrap(); | ||
| git(&work, &["add", "."]); | ||
| git(&work, &["commit", "-qm", "two"]); | ||
| git(&work, &["checkout", "-q", "main"]); | ||
|
|
||
| let bare_dir = root.join(owner_slug); | ||
| std::fs::create_dir_all(&bare_dir).unwrap(); | ||
| let bare = bare_dir.join("demo.git"); | ||
| git( | ||
| root, | ||
| &[ | ||
| "clone", | ||
| "-q", | ||
| "--bare", | ||
| work.to_str().unwrap(), | ||
| bare.to_str().unwrap(), | ||
| ], | ||
| ); | ||
|
|
||
| let tip = Command::new("git") | ||
| .args(["rev-parse", "refs/heads/feature"]) | ||
| .current_dir(&bare) | ||
| .output() | ||
| .unwrap(); | ||
| let tip = String::from_utf8_lossy(&tip.stdout).trim().to_string(); | ||
| assert_eq!(tip.len(), 40, "feature tip must resolve"); | ||
| (bare, tip) | ||
| } | ||
|
|
||
| /// Merging freezes the stored head at the source commit the merge actually | ||
| /// consumed. The PR row is loaded before the write lock is taken, so a push | ||
| /// that lands in between leaves the row's own `head_commit` stale; seeding a | ||
| /// value that matches nothing on disk stands in for that race, and the | ||
| /// merged PR must still come out pointing at the real `feature` tip. | ||
| #[sqlx::test] | ||
| async fn merge_stamps_the_source_head_it_merged_over_a_racing_value(pool: sqlx::PgPool) { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let owner_slug = OWNER_DID.replace([':', '/'], "_"); | ||
| let (bare, feature_tip) = bare_repo_with_feature(tmp.path(), &owner_slug); | ||
|
|
||
| let mut state = crate::test_support::test_state(pool.clone()).await; | ||
| state.repo_store = | ||
| crate::git::repo_store::RepoStore::for_testing(tmp.path().to_path_buf(), pool); | ||
|
|
||
| let now = Utc::now(); | ||
| let repo = crate::db::RepoRecord { | ||
| id: uuid::Uuid::new_v4().to_string(), | ||
| name: "demo".into(), | ||
| owner_did: OWNER_DID.into(), | ||
| description: None, | ||
| is_public: true, | ||
| default_branch: "main".into(), | ||
| created_at: now, | ||
| updated_at: now, | ||
| disk_path: bare.to_string_lossy().into_owned(), | ||
| forked_from: None, | ||
| machine_id: None, | ||
| }; | ||
| state.db.create_repo(&repo).await.unwrap(); | ||
|
|
||
| let pr = PullRequest { | ||
| id: Uuid::new_v4().to_string(), | ||
| repo_id: repo.id.clone(), | ||
| number: 1, | ||
| title: "add b".into(), | ||
| body: None, | ||
| author_did: OWNER_DID.into(), | ||
| source_branch: "feature".into(), | ||
| target_branch: "main".into(), | ||
| status: "open".into(), | ||
| merged_by_did: None, | ||
| merged_at: None, | ||
| head_commit: None, | ||
| created_at: now.to_rfc3339(), | ||
| updated_at: now.to_rfc3339(), | ||
| }; | ||
| state.db.create_pr(&pr).await.unwrap(); | ||
| // The stale value the racing push scenario leaves on the row. | ||
| state | ||
| .db | ||
| .set_open_pr_heads(&repo.id, "feature", STALE_SHA) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let db = state.db.clone(); | ||
| let app = Router::new() | ||
| .route( | ||
| "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", | ||
| post(merge_pr), | ||
| ) | ||
| .with_state(state); | ||
|
|
||
| let response = app | ||
| .oneshot(crate::test_support::signed_request_as( | ||
| OWNER_DID, | ||
| Method::POST, | ||
| "/api/v1/repos/z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH/demo/pulls/1/merge", | ||
| Body::empty(), | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(response.status(), StatusCode::OK); | ||
|
|
||
| let merged = db.get_pr(&repo.id, 1).await.unwrap().unwrap(); | ||
| assert_eq!(merged.status, "merged"); | ||
| assert_eq!( | ||
| merged.head_commit, | ||
| Some(feature_tip), | ||
| "the merge must stamp the source head it consumed, not the racing value" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// #173 F1 follow-up: merge holds no admission permit, so an exhausted write-lock | ||
| /// pool must shed 503 + Retry-After. | ||
| #[cfg(test)] | ||
|
|
@@ -480,6 +680,7 @@ mod lock_pool_shed_tests { | |
| state | ||
| .db | ||
| .create_pr(&PullRequest { | ||
| head_commit: None, | ||
| id: Uuid::new_v4().to_string(), | ||
| repo_id: repo_id.clone(), | ||
| number: 1, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.