Approvers are told; oh run approve and oh run reject dc59d1b56c3d
When a job that waits for approval becomes ready (its dependencies passed), everyone who may decide on it (the job's role, not the pusher with others-only, by the same rule the decision uses) gets a flow.approval notification, once. oh run approve N --job NAME and oh run reject N --job NAME call the API. Test: the approver is told once, the pusher and someone without the role are not. Refs andymccall/oxygenflow#57
- Author
- Andy McCall @andymccall <[email protected]>
- via
- agent
Claude Code <[email protected]> · delegate for changes, until 2026-11-13 19:00Z · key ea5f78160088
- When
- · 2026-10-16 04:19 UTC
- Signature
- ✔ signed by Claude Code
- Hash
dc59d1b56c3d2fe90077436d3acd62ff41bca271ff043b6c6ddbc69ba54aaa19- Depends on
- 070a7b64b368 2291f8cc9c96 417184c73f4e 42fa95540468 4a4400c5812d 5c6e92fd8d71 745d225de344
- Depended on by
- fac6853c4e46 05f1a85a2842 47e374020b59 7e01a7665aa5
- In bookmarks
- api-unknown-fields cancel-landed-runs diffusion-pin-bookmark draft-ready main oh-comment-body-file proposal-list-fast
- In tags
- v0.6.0 v0.6.1 v0.7.0 v0.8.0 v0.9.0 v0.9.1 v0.9.11 v0.9.15 v0.9.16 v0.9.19 v0.9.2 v0.9.20 v0.9.21 v0.9.22 v0.9.23 v0.9.24 v0.9.3 v0.9.4 v0.9.5 v0.9.6 v0.9.7 v0.9.8 v0.9.9
- Edits
- 8 atoms, 0 tree operations, 3 files
Browse the tree at this change
modified crates/hub/src/flow.rs +32 −0
| … | 225 unchanged lines |
||
| 226 | 226 | Ok(format!("{name}: {said}")) |
|
| 227 | 227 | } |
|
| 228 | 228 | ||
| 229 | + | /// Tell the people who may approve a job of `run` that has just become |
|
| 230 | + | /// ready (its dependencies all passed) and waits for them (oxygenflow#57). |
|
| 231 | + | /// Told once: the job's log notes it. |
|
| 232 | + | async fn tell_approvers(app: &Shared, row: &db::repos::RepoRow, run: &fdb::Run) { |
|
| 233 | + | const TOLD: &str = "waiting for approval; the people who may approve were told"; |
|
| 234 | + | let Ok(jobs) = fdb::jobs(&app.db, run.id).await else { return }; |
|
| 235 | + | for job in jobs.iter().filter(|j| j.status == "queued" && j.approval == "waiting") { |
|
| 236 | + | if !job.depends_on().iter().all(|d| jobs.iter().any(|j| &j.name == d && j.status == "passed")) { |
|
| 237 | + | continue; |
|
| 238 | + | } |
|
| 239 | + | let already = fdb::log(&app.db, job.id, 0, 50).await.unwrap_or_default().iter().any(|l| l.text == TOLD); |
|
| 240 | + | if already { |
|
| 241 | + | continue; |
|
| 242 | + | } |
|
| 243 | + | let mut targets = Vec::new(); |
|
| 244 | + | for a in db::accounts::accounts(&app.db).await.unwrap_or_default().iter().filter(|a| !a.blocked()) { |
|
| 245 | + | if may_decide(app, row, run, job, a).await.is_ok() { |
|
| 246 | + | targets.push((a.id, db::notify::Reason::Participating)); |
|
| 247 | + | } |
|
| 248 | + | } |
|
| 249 | + | let record = flow_core::api::Record { t: 0, stream: "flow".into(), text: TOLD.into() }; |
|
| 250 | + | fdb::append_log(&app.db, job.id, &[record], MAX_RECORDS as i64).await.ok(); |
|
| 251 | + | let title = format!("{}/{} run #{}: {} waits for your approval", row.owner, row.name, run.number, job.name); |
|
| 252 | + | let url = format!("/{}/{}/flow/{}", row.owner, row.name, run.number); |
|
| 253 | + | crate::notify::tell(app, "flow.approval", &title, &url, &run.actor, Some(row.id), targets).await; |
|
| 254 | + | } |
|
| 255 | + | } |
|
| 256 | + | ||
| 229 | 257 | /// End the approvals that waited too long (oxygenflow#57): from when the |
|
| 230 | 258 | /// jobs a waiting job depends on finished (or its run began). |
|
| 231 | 259 | pub async fn expire_approvals(app: &Shared, now: i64) -> sqlx::Result<usize> { |
|
| … | 1589 unchanged lines |
||
| 1821 | 1849 | return Ok(()); |
|
| 1822 | 1850 | }; |
|
| 1823 | 1851 | let mut errored_on: Vec<i64> = Vec::new(); |
|
| 1852 | + | let some_passed = changed.iter().any(|j| j.status == "passed"); |
|
| 1824 | 1853 | for j in changed { |
|
| 1825 | 1854 | let summary = match j.status.as_str() { |
|
| 1826 | 1855 | "passed" => format!("passed in {}", duration(&j)), |
|
| … | 12 unchanged lines |
||
| 1839 | 1868 | errored_on.dedup(); |
|
| 1840 | 1869 | for r in errored_on { |
|
| 1841 | 1870 | drain_if_erroring(app, r).await; |
|
| 1871 | + | } |
|
| 1872 | + | if some_passed { |
|
| 1873 | + | tell_approvers(app, &row, &run).await; |
|
| 1842 | 1874 | } |
|
| 1843 | 1875 | if let Some(done) = fdb::run_by_id(&app.db, run_id).await? { |
|
| 1844 | 1876 | if done.status == "failed" && !done.live() { |
|
| … | 465 unchanged lines |
modified crates/hub/tests/flow.rs +8 −0
| … | 4309 unchanged lines |
||
| 4310 | 4310 | assert!(early.contains("waits for plan first"), "{early}"); |
|
| 4311 | 4311 | let run = plan_passes(run.number).await; |
|
| 4312 | 4312 | assert!(claim(&h.app).await.is_none(), "apply waits for a person, so no runner gets it"); |
|
| 4313 | + | // Those who may approve are told, once; the pusher (others-only) and |
|
| 4314 | + | // someone without the role are not. |
|
| 4315 | + | let told = |id: i64| { |
|
| 4316 | + | let db = h.app.db.clone(); |
|
| 4317 | + | async move { db::notify::inbox(&db, id, 20).await.unwrap().into_iter().filter(|n| n.kind == "flow.approval").count() } |
|
| 4318 | + | }; |
|
| 4319 | + | assert_eq!((told(carol.id).await, told(andy.id).await, told(bob.id).await), (1, 0, 0)); |
|
| 4313 | 4320 | let (_, page) = call(&h.app, "GET", &format!("/andy/gated/flow/{}", run.number), None, None).await; |
|
| 4314 | 4321 | assert!(page.contains("waits for approval by someone with the <em>maintain</em> role, other than andy"), "the run page says so"); |
|
| 4315 | 4322 | let own = oxygenhub::flow::decide(&h.app, &row, &run, "apply", &andy, true).await.unwrap_err(); |
|
| … | 1 unchanged lines |
||
| 4317 | 4324 | let no_role = oxygenhub::flow::decide(&h.app, &row, &run, "apply", &bob, true).await.unwrap_err(); |
|
| 4318 | 4325 | assert!(no_role.contains("takes the maintain role"), "{no_role}"); |
|
| 4319 | 4326 | assert_eq!(oxygenhub::flow::decide(&h.app, &row, &run, "apply", &carol, true).await.unwrap(), "apply: approved by carol"); |
|
| 4327 | + | assert_eq!(told(carol.id).await, 1, "told once"); |
|
| 4320 | 4328 | let j = claim(&h.app).await.expect("approved, it runs"); |
|
| 4321 | 4329 | assert_eq!(j["job"]["name"], "apply"); |
|
| 4322 | 4330 | let (id, ph) = (j["job_id"].as_i64().unwrap(), j["pipeline_hash"].as_str().unwrap().to_string()); |
|
| … | 24 unchanged lines |
modified crates/oh/src/main.rs +25 −0
| … | 513 unchanged lines |
||
| 514 | 514 | }, |
|
| 515 | 515 | /// Cancel a run still queued or running; its running jobs are stopped |
|
| 516 | 516 | Cancel { number: i64 }, |
|
| 517 | + | /// Approve a job that waits for a person (oxygenflow#57) |
|
| 518 | + | Approve { |
|
| 519 | + | number: i64, |
|
| 520 | + | #[arg(long)] |
|
| 521 | + | job: String, |
|
| 522 | + | }, |
|
| 523 | + | /// Reject a job that waits for a person: it ends cancelled |
|
| 524 | + | Reject { |
|
| 525 | + | number: i64, |
|
| 526 | + | #[arg(long)] |
|
| 527 | + | job: String, |
|
| 528 | + | }, |
|
| 517 | 529 | } |
|
| 518 | 530 | ||
| 519 | 531 | #[derive(Subcommand)] |
|
| … | 560 unchanged lines |
||
| 1080 | 1092 | ||
| 1081 | 1093 | fn live(status: &str) -> bool { |
|
| 1082 | 1094 | matches!(status, "queued" | "running") |
|
| 1095 | + | } |
|
| 1096 | + | ||
| 1097 | + | /// `oh run approve|reject N --job NAME` (oxygenflow#57). |
|
| 1098 | + | fn decide_job(c: &Client, base: &str, number: i64, job: &str, verb: &str, json: bool) -> Result<()> { |
|
| 1099 | + | let done: serde_json::Value = c.send("POST", &format!("{base}/flow/runs/{number}/jobs/{job}/{verb}"), &serde_json::json!({}))?; |
|
| 1100 | + | if json { |
|
| 1101 | + | println!("{done}"); |
|
| 1102 | + | } else { |
|
| 1103 | + | println!("{} run #{number}: {}", ui::ok("✔"), done["said"].as_str().unwrap_or("")); |
|
| 1104 | + | } |
|
| 1105 | + | Ok(()) |
|
| 1083 | 1106 | } |
|
| 1084 | 1107 | ||
| 1085 | 1108 | fn watch(c: &Client, base: &str, number: i64, json: bool) -> Result<i32> { |
|
| … | 112 unchanged lines |
||
| 1198 | 1221 | println!("{} started run #{} {}", ui::ok("✔"), started["number"], ui::dim(started["url"].as_str().unwrap_or(""))); |
|
| 1199 | 1222 | } |
|
| 1200 | 1223 | } |
|
| 1224 | + | RunCmd::Approve { number, job } => decide_job(c, &base, number, &job, "approve", json)?, |
|
| 1225 | + | RunCmd::Reject { number, job } => decide_job(c, &base, number, &job, "reject", json)?, |
|
| 1201 | 1226 | RunCmd::Cancel { number } => { |
|
| 1202 | 1227 | let done: serde_json::Value = c.send("POST", &format!("{base}/flow/runs/{number}/cancel"), &serde_json::json!({}))?; |
|
| 1203 | 1228 | if json { |
|
| … | 386 unchanged lines |