Security and version updates open no issues on their own: an update whose version cannot be raised fails with why on its alert and is tried again a day later, and one whose required checks fail stays open for a person to decide on; an agent takes one only when a person asks with Fix with g1t. That issue now names the lockfiles the package is locked in and the commands that show it is done, which were left empty ("Locked in: .") because the lockfiles were never passed in.
11 files+75−2500/11 viewed
| 249 | 249 | or closes, whether g1t or a person closes it, g1t deletes that branch. If | |
| 250 | 250 | someone pushed to the branch after its last commit in the pull request, | |
| 251 | 251 | g1t leaves it alone. A branch left from an update pull request that is | |
| 252 | − | already closed is removed on a later push to the default branch. The | |
| 253 | − | branch of a pull request closed because code has to change is kept while | |
| 254 | − | g1t works on the issue for it. | |
| 252 | + | already closed is removed on a later push to the default branch. | |
| 255 | 253 | ||
| 256 | 254 | Deleting the branch means a closed update pull request cannot be reopened | |
| 257 | 255 | from the page. To make a version update again, comment `@g1t reopen` on | |
| ⋯ | |||
| 263 | 261 | [required checks](/guides/pull-requests/#required-status-checks) and the | |
| 264 | 262 | [merge queue](/guides/merge-queue/) like any other change. | |
| 265 | 263 | ||
| 266 | − | When a required check fails on g1t's own commit (or, on a branch with no | |
| 267 | − | required checks, a workflow's status fails), raising the version was not | |
| 268 | − | enough. g1t closes the pull request and opens an issue titled | |
| 269 | − | `<pull request title>: needs code changes`, assigned to | |
| 270 | − | [g1t](/guides/working-with-g1t/), to make the changes the update needs. | |
| 271 | − | Security updates are handled | |
| 272 | − | [the same way](/guides/security/#when-code-has-to-change). | |
| 264 | + | When a required check fails (or, on a branch with no required checks, a | |
| 265 | + | workflow's status fails), the pull request stays open with its failing | |
| 266 | + | checks for you to decide on: push the code changes the update needs to its | |
| 267 | + | branch, or close it. g1t opens no issue for it. Security updates are | |
| 268 | + | handled [the same way](/guides/security/#when-an-update-cannot-be-made). | |
| 273 | 269 | ||
| 274 | 270 | If the sandbox has not pushed a branch 45 minutes after an update started, | |
| 275 | 271 | the update is marked failed, and the entry's next run tries again. | |
| 336 | 336 | | Merged | The pull request merged. | | |
| 337 | 337 | | Closed | The pull request was closed without merging. | | |
| 338 | 338 | | Superseded | A newer security update for the same package replaced it, or the alerts it fixes were fixed another way or dismissed. | | |
| 339 | − | | Needs code changes | Raising the version was not enough; an agent is working on it. | | |
| 339 | + | | Needs code changes | An earlier update that g1t opened an issue for. New updates no longer reach this status. | | |
| 340 | 340 | | Failed | The update could not be made. | | |
| 341 | 341 | ||
| 342 | 342 | ### When an update is no longer needed | |
| ⋯ | |||
| 393 | 393 | ||
| 394 | 394 | Install scripts never run. A lockfile the sandbox cannot change this way, | |
| 395 | 395 | such as a `requirements.txt` that pins with `--hash`, is handled as | |
| 396 | − | [when code has to change](#when-code-has-to-change): if no branch has been | |
| 397 | − | pushed 45 minutes after the update started, g1t gets an issue for it. | |
| 396 | + | [when an update cannot be made](#when-an-update-cannot-be-made): if no | |
| 397 | + | branch has been pushed 45 minutes after the update started, the update | |
| 398 | + | fails. | |
| 398 | 399 | ||
| 399 | − | ### When code has to change | |
| 400 | + | ### When an update cannot be made | |
| 400 | 401 | ||
| 401 | − | Sometimes raising the version is not enough: the bump fails, or the pull | |
| 402 | − | request's required checks fail because code must change. Then g1t opens an | |
| 403 | − | issue and assigns it to [g1t](/guides/working-with-g1t/). That session | |
| 404 | − | shows as **started by g1t**. This is the only time an agent is involved in | |
| 405 | − | a security update. | |
| 402 | + | Sometimes raising the version is not enough, and g1t does not open an issue | |
| 403 | + | for it: | |
| 404 | + | ||
| 405 | + | - **The version cannot be raised.** A package that depends on it may hold | |
| 406 | + | it back, or the lockfile cannot be changed in the sandbox. The update is | |
| 407 | + | marked **Failed**, the alert says why, and g1t tries again a day later. | |
| 408 | + | - **The pull request's required checks fail.** It stays open with the | |
| 409 | + | failing checks for you to decide on: push the code changes it needs to | |
| 410 | + | its branch, or close it. | |
| 411 | + | ||
| 412 | + | To have an agent make the change, use **Fix with g1t** on the alert: it | |
| 413 | + | opens an issue and assigns it to [g1t](/guides/working-with-g1t/), as you. | |
| 414 | + | If the vulnerable code is never reached, dismiss the alert with a reason. | |
| 406 | 415 | ||
| 407 | 416 | Turn **Security updates** off under **Vulnerabilities** to stop g1t opening | |
| 408 | 417 | these pull requests for a project. Alerts are still listed. | |
| 396 | 396 | /// A newer security update replaced it, or the package is no longer | |
| 397 | 397 | /// vulnerable; g1t closed it. | |
| 398 | 398 | Superseded, | |
| 399 | − | /// The version could not be raised without changing code: an issue | |
| 400 | − | /// was opened for g1t instead. | |
| 399 | + | /// An earlier update g1t opened an issue for, when the version could | |
| 400 | + | /// not be raised without changing code. New updates no longer reach it: | |
| 401 | + | /// they fail with why, or their pull request stays open. | |
| 401 | 402 | NeedsCode, | |
| 402 | 403 | /// It could not be made; why is in `error`. | |
| 403 | 404 | Failed, |
| 160 | 160 | "pull.ready", | |
| 161 | 161 | "pull.merged", | |
| 162 | 162 | "pull.closed", | |
| 163 | − | "checks.completed", | |
| 164 | 163 | "comment.created", | |
| 165 | 164 | "repo.created", | |
| 166 | 165 | "repo.visibility_changed", |
| 147 | 147 | * Where a security update stands. `requested`: a sandbox is making the | |
| 148 | 148 | * change. `open`: its pull request is going through the required checks. | |
| 149 | 149 | * `superseded`: a newer update replaced it, or the package is no longer | |
| 150 | − | * vulnerable. `needs_code`: the version could not be raised without code | |
| 151 | − | * changes, so g1t has an issue for it. `failed`: see `error`. | |
| 150 | + | * vulnerable. `needs_code`: an earlier update g1t opened an issue for; new | |
| 151 | + | * updates no longer reach it. `failed`: see `error`. | |
| 152 | 152 | */ | |
| 153 | 153 | export type UpdateState = "requested" | "open" | "merged" | "closed" | "superseded" | "needs_code" | "failed"; | |
| 154 | 154 |
| 81 | 81 | /// One package in one lockfile. | |
| 82 | 82 | pub(crate) struct Located { | |
| 83 | 83 | pub(crate) package: Package, | |
| 84 | − | pub(crate) lockfile: Lockfile, | |
| 85 | 84 | pub(crate) path: String, | |
| 86 | 85 | } | |
| 87 | 86 | ||
| ⋯ | |||
| 101 | 100 | continue; | |
| 102 | 101 | } | |
| 103 | 102 | for package in lockfile.parse(&file.text) { | |
| 104 | − | located.push(Located { package, lockfile, path: file.path.clone() }); | |
| 103 | + | located.push(Located { package, path: file.path.clone() }); | |
| 105 | 104 | } | |
| 106 | 105 | } | |
| 107 | 106 | located | |
| ⋯ | |||
| 382 | 381 | } | |
| 383 | 382 | } | |
| 384 | 383 | ||
| 385 | − | /// The issue's body, for when raising the version is not enough, written | |
| 386 | − | /// for the agent that takes it as much as for a person, ending with what | |
| 387 | − | /// done means: commands that show no lockfile still resolves a vulnerable | |
| 388 | − | /// version, and the project's tests. The pull request merges on the | |
| 389 | − | /// repository's required checks, like any other. | |
| 390 | − | pub(crate) fn issue_text(ecosystem: &str, package: &str, target: &str, vulns: &[&VulnRow], located: &[Located]) -> String { | |
| 384 | + | /// The issue's body, for when a person asks g1t to fix a vulnerability | |
| 385 | + | /// alert, written for the agent that takes it as much as for a person, | |
| 386 | + | /// ending with what done means: commands that show no lockfile still | |
| 387 | + | /// resolves a vulnerable version, and the project's tests. The pull request | |
| 388 | + | /// merges on the repository's required checks, like any other. | |
| 389 | + | pub(crate) fn issue_text(ecosystem: &str, package: &str, target: &str, vulns: &[&VulnRow]) -> String { | |
| 391 | 390 | let mut body = format!( | |
| 392 | − | "`{package}` ({ecosystem}) has known vulnerabilities with a fix in **{target}**, and raising its version alone \ | |
| 393 | − | does not pass this project's checks. Upgrade it to {target} or later everywhere it is locked, and change the \ | |
| 394 | − | code that depends on it, keeping other changes to what the upgrade needs.\n\n" | |
| 391 | + | "`{package}` ({ecosystem}) has known vulnerabilities with a fix in **{target}**. Upgrade it to {target} or later \ | |
| 392 | + | everywhere it is locked, and change the code that depends on it where the upgrade needs that, keeping other \ | |
| 393 | + | changes to what the upgrade needs.\n\n" | |
| 395 | 394 | ); | |
| 396 | 395 | body.push_str(&advisory_table(vulns)); | |
| 397 | 396 | let mut checks = Vec::new(); | |
| 398 | 397 | let mut manifests = BTreeSet::new(); | |
| 399 | 398 | let mut tests = BTreeSet::new(); | |
| 400 | 399 | for vuln in vulns { | |
| 401 | − | let Some(item) = located | |
| 402 | − | .iter() | |
| 403 | − | .find(|item| item.path == vuln.manifest && item.package.name == vuln.package && item.package.version == vuln.version) | |
| 404 | − | else { | |
| 405 | − | continue; | |
| 406 | − | }; | |
| 407 | − | if manifests.insert((item.path.clone(), item.package.version.clone())) { | |
| 408 | − | checks.push(still_locked_check(item.lockfile, &item.path, &item.package.name, &item.package.version)); | |
| 400 | + | let Some(lockfile) = Lockfile::for_path(&vuln.manifest) else { continue }; | |
| 401 | + | if manifests.insert((vuln.manifest.clone(), vuln.version.clone())) { | |
| 402 | + | checks.push(still_locked_check(lockfile, &vuln.manifest, &vuln.package, &vuln.version)); | |
| 409 | 403 | } | |
| 410 | − | if let Some(test) = test_command(item.lockfile, directory(&item.path)) { | |
| 404 | + | if let Some(test) = test_command(lockfile, directory(&vuln.manifest)) { | |
| 411 | 405 | tests.insert(test); | |
| 412 | 406 | } | |
| 413 | 407 | } | |
| ⋯ | |||
| 498 | 492 | dismissed_by: None, | |
| 499 | 493 | dismissed_at: None, | |
| 500 | 494 | }; | |
| 501 | − | let located = vec![Located { | |
| 502 | − | package: Package { ecosystem: g1t_scan::lockfiles::Ecosystem::Npm, name: "lodash".into(), version: "4.17.20".into() }, | |
| 503 | − | lockfile: Lockfile::PackageLock, | |
| 504 | − | path: "web/package-lock.json".into(), | |
| 505 | − | }]; | |
| 506 | − | let body = issue_text("npm", "lodash", "4.17.21", &[&row], &located); | |
| 495 | + | let body = issue_text("npm", "lodash", "4.17.21", &[&row]); | |
| 507 | 496 | assert!(body.contains("[GHSA-35jh-r3h4-6jhm](https://osv.dev/vulnerability/GHSA-35jh-r3h4-6jhm) | high | 4.17.20 | 4.17.21")); | |
| 508 | 497 | assert!(body.contains("`web/package-lock.json` (4.17.20)")); | |
| 509 | 498 | let (_, done) = body.split_once("## Definition of done\n\n").unwrap(); | |
| 160 | 160 | let open = self.store.open_vulnerabilities(repo_id).await?; | |
| 161 | 161 | let rows: Vec<&VulnRow> = open.iter().filter(|row| row.ecosystem == vuln.ecosystem && row.package == vuln.package).collect(); | |
| 162 | 162 | let target = vuln.fixed_version.clone().unwrap_or_else(|| "a version without these advisories".to_owned()); | |
| 163 | − | let body = crate::deps::issue_text(&vuln.ecosystem, &vuln.package, &target, &rows, &[]); | |
| 163 | + | let body = crate::deps::issue_text(&vuln.ecosystem, &vuln.package, &target, &rows); | |
| 164 | 164 | Ok((format!("Upgrade {} to {target}", vuln.package), body)) | |
| 165 | 165 | } | |
| 166 | 166 | } |
| 118 | 118 | unscanned: bool, | |
| 119 | 119 | } | |
| 120 | 120 | ||
| 121 | − | /// `pull.merged`, `pull.closed` and `checks.completed`, as far as this | |
| 122 | − | /// service reads them. | |
| 121 | + | /// `pull.merged` and `pull.closed`, as far as this service reads them. | |
| 123 | 122 | #[derive(Deserialize)] | |
| 124 | 123 | #[serde(rename_all = "camelCase")] | |
| 125 | 124 | struct PullHappened { | |
| 126 | 125 | repo_id: String, | |
| 127 | 126 | number: u32, | |
| 128 | − | #[serde(default)] | |
| 129 | − | status: Option<String>, | |
| 130 | 127 | } | |
| 131 | 128 | ||
| 132 | 129 | /// The longest comment a dismissal keeps. | |
| ⋯ | |||
| 569 | 566 | } | |
| 570 | 567 | } | |
| 571 | 568 | } | |
| 572 | − | "pull.merged" | "pull.closed" | "checks.completed" => { | |
| 573 | − | if let Ok(happened) = serde_json::from_value::<PullHappened>(event.data.clone()) | |
| 574 | − | && !(event.kind != "checks.completed" && self.update_pull_closed(&event.kind, &happened.repo_id, happened.number).await?) | |
| 575 | − | { | |
| 576 | − | self.update_pull_event( | |
| 577 | − | &event.kind, | |
| 578 | − | &happened.repo_id, | |
| 579 | − | happened.number, | |
| 580 | − | happened.status.as_deref(), | |
| 581 | − | event.actor.as_deref(), | |
| 582 | − | ) | |
| 583 | − | .await?; | |
| 584 | − | } | |
| 585 | − | // One of g1t's update pull requests closed or merged, by g1t | |
| 586 | − | // or by a person: its branch goes. | |
| 587 | − | if event.kind != "checks.completed" | |
| 588 | − | && let Ok(happened) = serde_json::from_value::<PullHappened>(event.data.clone()) | |
| 589 | − | { | |
| 569 | + | "pull.merged" | "pull.closed" => { | |
| 570 | + | if let Ok(happened) = serde_json::from_value::<PullHappened>(event.data.clone()) { | |
| 571 | + | if !self.update_pull_closed(&event.kind, &happened.repo_id, happened.number).await? { | |
| 572 | + | self.update_pull_event(&event.kind, &happened.repo_id, happened.number, event.actor.as_deref()).await?; | |
| 573 | + | } | |
| 574 | + | // One of g1t's update pull requests closed or merged, by | |
| 575 | + | // g1t or by a person: its branch goes. | |
| 590 | 576 | self.pull_finished(&happened.repo_id, happened.number).await?; | |
| 591 | 577 | } | |
| 592 | 578 | } | |
| 483 | 483 | ||
| 484 | 484 | fn located(name: &str, version: &str, path: &str) -> Located { | |
| 485 | 485 | let lockfile = Lockfile::for_path(path).unwrap(); | |
| 486 | − | Located { package: Package { name: name.into(), version: version.into(), ecosystem: lockfile.ecosystem() }, lockfile, path: path.into() } | |
| 486 | + | Located { package: Package { name: name.into(), version: version.into(), ecosystem: lockfile.ecosystem() }, path: path.into() } | |
| 487 | 487 | } | |
| 488 | 488 | ||
| 489 | 489 | #[test] |
| 22 | 22 | use g1t_contracts::repos::RepoPath; | |
| 23 | 23 | use g1t_contracts::security::{BumpArgs, UpdateState, update_branch}; | |
| 24 | 24 | use g1t_contracts::time::rfc3339; | |
| 25 | − | use g1t_contracts::work::{OpenIssueArgs, OpenPullArgs, Pull, PullActionArgs, PullDetail, PullStatus, Runtime, ViewArgs}; | |
| 25 | + | use g1t_contracts::work::{OpenPullArgs, Pull, PullActionArgs, PullDetail, PullStatus, Runtime, ViewArgs}; | |
| 26 | 26 | use g1t_contracts::{Outcome, User}; | |
| 27 | 27 | use g1t_kit::now_ms; | |
| 28 | 28 | use g1t_scan::osv::{self, Severity}; | |
| 29 | 29 | use g1t_scan::version; | |
| 30 | − | use serde_json::{Value, json}; | |
| 30 | + | use serde_json::Value; | |
| 31 | 31 | use worker::Result; | |
| 32 | 32 | ||
| 33 | 33 | use g1t_contracts::security::UPDATE_BRANCH_PREFIX; | |
| ⋯ | |||
| 36 | 36 | ||
| 37 | 37 | use crate::Security; | |
| 38 | 38 | use crate::config::{CommitMessage, Config, Entry, glob, matches}; | |
| 39 | − | use crate::deps::{advisory_table, issue_text}; | |
| 39 | + | use crate::deps::advisory_table; | |
| 40 | 40 | use crate::pull_text; | |
| 41 | 41 | use crate::ranges; | |
| 42 | 42 | use crate::store::{Activity, RepoRow, UpdateRow, VulnRow}; | |
| ⋯ | |||
| 580 | 580 | self.note(&row, "update_opened", Some(g1t_contracts::system::USERNAME), Some(pull.number), None).await | |
| 581 | 581 | } | |
| 582 | 582 | ||
| 583 | − | /// A pull request merged, closed or checked: if it is a security | |
| 584 | − | /// update's, where the update stands now. | |
| 585 | − | pub async fn update_pull_event(&self, kind: &str, repo_id: &str, number: u32, status: Option<&str>, actor: Option<&str>) -> Result<()> { | |
| 583 | + | /// A pull request merged or closed: if it is a security update's, where | |
| 584 | + | /// the update stands now. One whose checks fail stays open for a person | |
| 585 | + | /// to decide on. | |
| 586 | + | pub async fn update_pull_event(&self, kind: &str, repo_id: &str, number: u32, actor: Option<&str>) -> Result<()> { | |
| 586 | 587 | let Some(row) = self.store.update_by_pull(repo_id, number).await? else { | |
| 587 | 588 | return Ok(()); | |
| 588 | 589 | }; | |
| ⋯ | |||
| 598 | 599 | self.store.set_update(&row, UpdateState::Closed, Some(number), None, None).await?; | |
| 599 | 600 | self.note(&row, "update_closed", actor, Some(number), None).await | |
| 600 | 601 | } | |
| 601 | − | "checks.completed" if status == Some("failed") => { | |
| 602 | − | let Some(repo) = self.store.repo(repo_id).await? else { | |
| 603 | − | return Ok(()); | |
| 604 | − | }; | |
| 605 | − | self.needs_code(&repo, &row, "Raising the version alone fails this branch's required checks").await | |
| 606 | − | } | |
| 607 | 602 | _ => Ok(()), | |
| 608 | − | } | |
| 609 | − | } | |
| 610 | − | ||
| 611 | − | /// Closes an update that is no longer needed: its pull request, if it | |
| 612 | − | /// has one still open (one that merged meanwhile is recorded merged). | |
| 613 | − | async fn supersede(&self, repo: &RepoRow, row: &UpdateRow, why: String) -> Result<()> { | |
| 614 | − | if let Some(number) = row.pull() { | |
| 615 | − | match self.get_pull(repo, number).await? { | |
| 616 | − | Some(pull) if pull.status == PullStatus::Merged => { | |
| 617 | − | return self.store.set_update(row, UpdateState::Merged, Some(number), row.issue(), None).await; | |
| 618 | − | } | |
| 619 | − | Some(pull) if matches!(pull.status, PullStatus::Open | PullStatus::Draft) => { | |
| 620 | − | self.store.set_update(row, UpdateState::Superseded, Some(number), row.issue(), None).await?; | |
| 621 | − | self.close_with(repo, number, why).await?; | |
| 622 | − | return self.note(row, "update_superseded", Some(g1t_contracts::system::USERNAME), Some(number), None).await; | |
| 623 | − | } | |
| 624 | − | _ => {} | |
| 625 | − | } | |
| 626 | 603 | } | |
| 627 | − | self.store.set_update(row, UpdateState::Superseded, row.pull(), row.issue(), None).await | |
| 628 | 604 | } | |
| 629 | 605 | ||
| 630 | 606 | /// Updates whose sandbox never pushed: raising the version did not | |
| 631 | − | /// work, so g1t gets an issue for it. On a repository that takes no | |
| 632 | − | /// updates now (security updates off, archived, deleted, or a mirror | |
| 633 | − | /// that is not taken over, whose pushes are refused) the update fails | |
| 634 | − | /// with the reason instead: nothing is opened there. | |
| 607 | + | /// work on its own, so the update fails with why, shown on the alert, | |
| 608 | + | /// and is tried again later. Nothing is opened for it: a person can ask | |
| 609 | + | /// g1t to fix the alert. On a repository that takes no updates now | |
| 610 | + | /// (security updates off, archived, deleted, or a mirror that is not | |
| 611 | + | /// taken over, whose pushes are refused) the reason is that instead. | |
| 635 | 612 | pub async fn stalled_updates(&self) -> Result<()> { | |
| 636 | 613 | let before = rfc3339(now_ms().saturating_sub(STALLED_MS)); | |
| 637 | 614 | for row in self.store.stalled_updates(&before, 10).await? { | |
| ⋯ | |||
| 641 | 618 | self.store.set_update(&row, UpdateState::Failed, row.pull(), None, Some(&why)).await?; | |
| 642 | 619 | continue; | |
| 643 | 620 | } | |
| 644 | − | self.needs_code(&repo, &row, "The version could not be raised in a sandbox on its own").await?; | |
| 621 | + | let why = format!( | |
| 622 | + | "`{}` could not be raised to {} on its own: a package that depends on it may hold it back, or the upgrade needs code changes.", | |
| 623 | + | row.package, row.target | |
| 624 | + | ); | |
| 625 | + | self.store.set_update(&row, UpdateState::Failed, row.pull(), None, Some(&why)).await?; | |
| 626 | + | self.note(&row, "update_failed", Some(g1t_contracts::system::USERNAME), None, Some(&why)).await?; | |
| 645 | 627 | } | |
| 646 | 628 | Ok(()) | |
| 647 | − | } | |
| 648 | − | ||
| 649 | − | /// Raising the version is not enough: closes g1t's pull request, opens | |
| 650 | − | /// an issue for the change, and puts g1t to work on it. | |
| 651 | − | async fn needs_code(&self, repo: &RepoRow, row: &UpdateRow, why: &str) -> Result<()> { | |
| 652 | − | let path = Self::path_of(repo); | |
| 653 | − | let system = User::system(&repo.namespace); | |
| 654 | − | let open = self.store.open_vulnerabilities(&repo.repo_id).await?; | |
| 655 | − | let vulns: Vec<&VulnRow> = open | |
| 656 | − | .iter() | |
| 657 | − | .filter(|vuln| vuln.ecosystem == row.ecosystem && vuln.package == row.package) | |
| 658 | − | .collect(); | |
| 659 | − | if vulns.is_empty() { | |
| 660 | − | return self.supersede(repo, row, format!("`{}` is no longer vulnerable here.", row.package)).await; | |
| 661 | − | } | |
| 662 | − | let issue: Outcome<g1t_contracts::work::Issue> = g1t_kit::call( | |
| 663 | − | &self.work, | |
| 664 | − | "open_issue", | |
| 665 | − | &OpenIssueArgs { | |
| 666 | − | actor: system.clone(), | |
| 667 | − | repo: path.clone(), | |
| 668 | − | title: format!("Upgrade {} to {}: needs code changes", row.package, row.target).chars().take(200).collect(), | |
| 669 | − | body: issue_text(&row.ecosystem, &row.package, &row.target, &vulns, &[]), | |
| 670 | − | labels: vec!["dependencies".to_owned(), "security".to_owned()], | |
| 671 | − | checks: Vec::new(), | |
| 672 | − | milestone: None, | |
| 673 | − | }, | |
| 674 | − | ) | |
| 675 | − | .await?; | |
| 676 | − | let issue = match issue { | |
| 677 | − | Outcome::Ok(issue) => issue, | |
| 678 | − | Outcome::Fail(refused) => { | |
| 679 | − | let error = format!("{why}, and the issue for it could not be opened: {}", refused.message); | |
| 680 | − | self.store.set_update(row, UpdateState::Failed, row.pull(), None, Some(&error)).await?; | |
| 681 | − | return self.note(row, "update_failed", Some(g1t_contracts::system::USERNAME), None, Some(&error)).await; | |
| 682 | − | } | |
| 683 | − | }; | |
| 684 | − | self.store | |
| 685 | − | .set_update(row, UpdateState::NeedsCode, row.pull(), Some(issue.number), Some(why)) | |
| 686 | − | .await?; | |
| 687 | − | if let Some(number) = row.pull() { | |
| 688 | − | self.close_with(repo, number, format!("{why}, so code has to change too. g1t is making the change in #{}.", issue.number)) | |
| 689 | − | .await?; | |
| 690 | − | } | |
| 691 | − | let started: Outcome<Value> = | |
| 692 | − | g1t_kit::call(&self.runner, "run", &json!({ "actor": system, "repo": path, "issue": issue.number })).await?; | |
| 693 | − | if let Outcome::Fail(refused) = started { | |
| 694 | − | self.comment(&system, &path, issue.number, format!( | |
| 695 | − | "g1t could not put an agent on this upgrade: {}\n\nAssign it to g1t once agents can run here, or upgrade it by hand.", | |
| 696 | − | refused.message | |
| 697 | − | )) | |
| 698 | − | .await?; | |
| 699 | − | } | |
| 700 | − | self.note(row, "update_needs_code", Some(g1t_contracts::system::USERNAME), Some(issue.number), Some(why)).await | |
| 701 | 629 | } | |
| 702 | 630 | } | |
| 703 | 631 | ||
| 35 | 35 | VersionUpdatesState, update_command, | |
| 36 | 36 | }; | |
| 37 | 37 | use g1t_contracts::work::{ | |
| 38 | − | Mergeable, OpenIssueArgs, OpenPullArgs, Pull, PullActionArgs, PullDetail, PullStatus, RequiredState, Runtime, SetCommitStatusArgs, | |
| 38 | + | Mergeable, OpenPullArgs, Pull, PullActionArgs, PullDetail, PullStatus, RequiredState, Runtime, SetCommitStatusArgs, | |
| 39 | 39 | UpdatePullArgs, ViewArgs, | |
| 40 | 40 | }; | |
| 41 | 41 | use g1t_contracts::{FailureCode, Outcome, User}; | |
| ⋯ | |||
| 43 | 43 | use g1t_scan::lockfiles::Lockfile; | |
| 44 | 44 | use g1t_scan::version; | |
| 45 | 45 | use serde::Deserialize; | |
| 46 | − | use serde_json::{Value, json}; | |
| 46 | + | use serde_json::Value; | |
| 47 | 47 | use worker::{Fetch, Headers, Method, Request, RequestInit, Result}; | |
| 48 | 48 | ||
| 49 | 49 | use crate::Security; | |
| ⋯ | |||
| 1051 | 1051 | } | |
| 1052 | 1052 | let ours = row.head.is_some() && row.head == detail.pull.head_commit; | |
| 1053 | 1053 | match checks_verdict(&detail) { | |
| 1054 | − | Some(false) if ours => return self.update_needs_code(&repo, row, &detail).await, | |
| 1055 | 1054 | Some(true) if row.merge_by.is_some() => { | |
| 1056 | 1055 | let merged: Outcome<Pull> = g1t_kit::call( | |
| 1057 | 1056 | &self.work, | |
| ⋯ | |||
| 1097 | 1096 | .unwrap_or_else(|| "auto".to_owned()) | |
| 1098 | 1097 | } | |
| 1099 | 1098 | ||
| 1100 | − | /// An update that breaks the branch's required checks: closed, and an | |
| 1101 | − | /// issue opened for g1t to make the code changes it needs. | |
| 1102 | − | async fn update_needs_code(&self, repo: &RepoRow, row: &PullRow, detail: &PullDetail) -> Result<()> { | |
| 1103 | − | let system = User::system(&repo.namespace); | |
| 1104 | − | let path = Self::repo_path(repo); | |
| 1105 | − | let failing: Vec<String> = if detail.required_checks.is_empty() { | |
| 1106 | − | detail.statuses.iter().filter(|status| matches!(status.state.as_str(), "failure" | "error")).map(|status| status.context.clone()).collect() | |
| 1107 | − | } else { | |
| 1108 | − | detail.required_checks.iter().filter(|check| check.state == RequiredState::Failure).map(|check| check.name.clone()).collect() | |
| 1109 | − | }; | |
| 1110 | − | let issue: Outcome<g1t_contracts::work::Issue> = g1t_kit::call( | |
| 1111 | − | &self.work, | |
| 1112 | − | "open_issue", | |
| 1113 | − | &OpenIssueArgs { | |
| 1114 | − | actor: system.clone(), | |
| 1115 | − | repo: path.clone(), | |
| 1116 | − | title: format!("{}: needs code changes", row.title).chars().take(200).collect(), | |
| 1117 | − | body: needs_code_text(row, &failing, detail.pull.number), | |
| 1118 | − | labels: vec!["dependencies".to_owned()], | |
| 1119 | − | checks: Vec::new(), | |
| 1120 | − | milestone: None, | |
| 1121 | − | }, | |
| 1122 | − | ) | |
| 1123 | − | .await?; | |
| 1124 | − | let issue = match issue { | |
| 1125 | − | Outcome::Ok(issue) => issue, | |
| 1126 | − | Outcome::Fail(refused) => { | |
| 1127 | − | let error = format!("Its checks fail, and the issue for it could not be opened: {}", refused.message); | |
| 1128 | − | return self.store.set_update_pull(&row.id, UpdateState::Failed, row.pull(), None, Some(&error)).await; | |
| 1129 | − | } | |
| 1130 | − | }; | |
| 1131 | − | self.store.set_update_pull(&row.id, UpdateState::NeedsCode, row.pull(), Some(issue.number), Some("Raising the versions fails this branch's required checks.")).await?; | |
| 1132 | − | self.close_with(repo, detail.pull.number, format!("Raising the versions alone fails this branch's required checks, so code has to change too. g1t is making the change in #{}.", issue.number)) | |
| 1133 | − | .await?; | |
| 1134 | − | let started: Outcome<Value> = g1t_kit::call(&self.runner, "run", &json!({ "actor": system, "repo": path, "issue": issue.number })).await?; | |
| 1135 | − | if let Outcome::Fail(refused) = started { | |
| 1136 | − | self.comment(&system, &path, issue.number, format!( | |
| 1137 | − | "g1t could not put an agent on this update: {}\n\nAssign it to g1t once agents can run here, or make the change by hand.", | |
| 1138 | − | refused.message | |
| 1139 | − | )) | |
| 1140 | − | .await?; | |
| 1141 | − | } | |
| 1142 | − | Ok(()) | |
| 1143 | − | } | |
| 1144 | − | ||
| 1145 | 1099 | /// A comment on an update pull request: a command, if it is one. | |
| 1146 | 1100 | pub async fn update_comment(&self, event: &Event) -> Result<()> { | |
| 1147 | 1101 | #[derive(Deserialize)] | |
| ⋯ | |||
| 1518 | 1472 | directory: update.directory.clone(), | |
| 1519 | 1473 | dependency_type: update.kind.label().to_owned(), | |
| 1520 | 1474 | update_type: format!("version-update:semver-{}", update.level), | |
| 1521 | − | } | |
| 1522 | − | } | |
| 1523 | − | ||
| 1524 | − | /// The issue for an update that breaks the build, for the agent that takes | |
| 1525 | − | /// it as much as for a person. | |
| 1526 | − | pub fn needs_code_text(row: &PullRow, failing: &[String], pull: u32) -> String { | |
| 1527 | − | let mut body = format!( | |
| 1528 | − | "#{pull} raises these dependencies, and this branch's required checks fail with only the versions changed{}:\n\n| Package | Directory | From | To |\n| --- | --- | --- | --- |\n", | |
| 1529 | − | if failing.is_empty() { String::new() } else { format!(" ({})", failing.join(", ")) } | |
| 1530 | − | ); | |
| 1531 | − | for dependency in row.dependencies() { | |
| 1532 | − | body.push_str(&format!("| `{}` | `{}` | {} | {} |\n", dependency.name, dependency.directory, dependency.from, dependency.to)); | |
| 1533 | 1475 | } | |
| 1534 | − | body.push_str( | |
| 1535 | − | "\nUpgrade them as #{pull} does and change the code that depends on them until the checks pass, keeping other changes to what the upgrade needs. Read each package's release notes for what changed.", | |
| 1536 | − | ); | |
| 1537 | − | let body = body.replace("#{pull}", &format!("#{pull}")); | |
| 1538 | − | let mut body = g1t_contracts::work::with_definition_of_done(&body, &["Every dependency above is at its new version, and the required checks pass.".to_owned()]); | |
| 1539 | − | body.push_str("\n\n---\n_Opened by g1t's version updates._"); | |
| 1540 | − | body | |
| 1541 | 1476 | } | |
| 1542 | 1477 | ||
| 1543 | 1478 | #[cfg(test)] | |
| ⋯ | |||
| 1545 | 1480 | use super::*; | |
| 1546 | 1481 | use crate::config::read; | |
| 1547 | 1482 | use g1t_contracts::work::{CommitStatus, RequiredCheck}; | |
| 1483 | + | use serde_json::json; | |
| 1548 | 1484 | ||
| 1549 | 1485 | fn entry(extra: &str) -> Entry { | |
| 1550 | 1486 | let found = read(&format!("version: 2\nupdates:\n - package-ecosystem: npm\n{extra} schedule: {{interval: daily}}\n")); | |
| ⋯ | |||
| 1710 | 1646 | assert_eq!(target_of(&entry(" directory: /\n"), "main"), None); | |
| 1711 | 1647 | assert_eq!(osv_ecosystem("gomod"), Some("Go")); | |
| 1712 | 1648 | assert_eq!(package_ecosystem("crates.io"), Some("cargo")); | |
| 1713 | − | } | |
| 1714 | − | ||
| 1715 | − | #[test] | |
| 1716 | − | fn the_issue_names_what_broke() { | |
| 1717 | − | let mut found = row("group:lint", "", "open"); | |
| 1718 | − | found.dependencies = serde_json::to_string(&[UpdatedDependency { | |
| 1719 | − | name: "eslint".into(), | |
| 1720 | − | from: "8.0.0".into(), | |
| 1721 | − | to: "9.0.0".into(), | |
| 1722 | − | directory: "/".into(), | |
| 1723 | − | dependency_type: "direct:development".into(), | |
| 1724 | − | update_type: "version-update:semver-major".into(), | |
| 1725 | − | }]) | |
| 1726 | − | .unwrap(); | |
| 1727 | − | let text = needs_code_text(&found, &["CI".into()], 12); | |
| 1728 | − | assert!(text.starts_with("#12 raises these dependencies, and this branch's required checks fail with only the versions changed (CI):")); | |
| 1729 | − | assert!(text.contains("| `eslint` | `/` | 8.0.0 | 9.0.0 |")); | |
| 1730 | − | assert!(text.contains("Upgrade them as #12 does")); | |
| 1731 | − | assert!(text.contains("## Definition of done")); | |
| 1732 | 1649 | } | |
| 1733 | 1650 | } | |