Security keeps work items, not issue numbers
The work items g1t opened for upgrades before security updates, and the alert activity that named them, are rewritten from the old issue numbers to the work items' numbers in the workspace when work.migrated is heard. Old upgrade work items are read and closed through the work item RPCs.
- G1T-Session:
- ses_01m566357hejh9yeqjbetx23f3
14 files+224−830/14 viewed
| 1681 | 1681 | "g1t-kit", | |
| 1682 | 1682 | "g1t-scan", | |
| 1683 | 1683 | "getrandom 0.2.17", | |
| 1684 | + | "rusqlite", | |
| 1684 | 1685 | "serde", | |
| 1685 | 1686 | "serde_json", | |
| 1686 | 1687 | "serde_yaml", |
| 23 | 23 | futures-util = { version = "0.3", default-features = false, features = ["alloc"] } | |
| 24 | 24 | getrandom = { version = "0.2", features = ["js"] } | |
| 25 | 25 | ||
| 26 | + | [dev-dependencies] | |
| 27 | + | # The migrations and the SQL that rewrites rows, on SQLite as D1 runs it. | |
| 28 | + | rusqlite = { version = "0.32", features = ["bundled"] } | |
| 29 | + | ||
| 26 | 30 | # wasm-opt at -O1: about the same gzipped size as -O in a tenth of the | |
| 27 | 31 | # time. | |
| 28 | 32 | [package.metadata.wasm-pack.profile.release] |
| 1 | + | -- Issues became work items of the workspace (g1t #57). What g1t opened | |
| 2 | + | -- for an upgrade, and what it was put on to fix an alert, is now a work | |
| 3 | + | -- item, by its number in the repository's workspace. | |
| 4 | + | -- | |
| 5 | + | -- Until the work service has numbered the moved issues, these columns | |
| 6 | + | -- still hold the old issue numbers of each row's repository: `work.migrated` | |
| 7 | + | -- rewrites them (lib.rs `work_migrated`). Every new number in a workspace | |
| 8 | + | -- is above every old number of its repositories, so a rewritten value is | |
| 9 | + | -- never taken for an old one. | |
| 10 | + | ALTER TABLE updates RENAME COLUMN issue TO item; | |
| 11 | + | ALTER TABLE update_pulls RENAME COLUMN issue TO item; | |
| 12 | + | ALTER TABLE code_alerts RENAME COLUMN issue TO item; | |
| 13 | + | ALTER TABLE alert_fixes RENAME COLUMN issue TO item; | |
| 14 | + | ALTER TABLE upgrades RENAME COLUMN number TO item; |
| 492 | 492 | "add_comment", | |
| 493 | 493 | &AddCommentArgs { | |
| 494 | 494 | actor: system.clone(), | |
| 495 | − | repo: path.clone(), | |
| 495 | + | repo: Some(path.clone()), | |
| 496 | + | workspace: String::new(), | |
| 496 | 497 | number, | |
| 497 | 498 | body: comment_text(&result.tool, result, &page), | |
| 498 | 499 | path: result.path.clone(), |
| 9 | 9 | use g1t_contracts::security::{AlertState, FindLockfilesArgs, Lockfiles, VersionUpdatesState, VulnStatus, Vulnerability}; | |
| 10 | 10 | use g1t_contracts::updates::DEPENDABOT_PATHS; | |
| 11 | 11 | use g1t_contracts::time::rfc3339; | |
| 12 | − | use g1t_contracts::work::{AddCommentArgs, Issue, IssueDetail, ViewArgs}; | |
| 12 | + | use g1t_contracts::work::{AddCommentArgs, WorkItem, WorkItemDetail, WorkItemViewArgs}; | |
| 13 | 13 | use g1t_contracts::{Outcome, User}; | |
| 14 | 14 | use g1t_kit::now_ms; | |
| 15 | 15 | use g1t_scan::lockfiles::{Lockfile, Package}; | |
| ⋯ | |||
| 271 | 271 | Ok(config.map(|(config, file)| Loaded { config, file, default_branch: default_branch.unwrap_or_default() })) | |
| 272 | 272 | } | |
| 273 | 273 | ||
| 274 | − | pub(crate) async fn issue(&self, actor: &User, repo: &RepoPath, number: u32) -> Result<Option<Issue>> { | |
| 275 | − | let found: Outcome<IssueDetail> = g1t_kit::call( | |
| 274 | + | /// A work item of `workspace`, as `actor` sees it. | |
| 275 | + | pub(crate) async fn item(&self, actor: &User, workspace: &str, number: u32) -> Result<Option<WorkItem>> { | |
| 276 | + | let found: Outcome<WorkItemDetail> = g1t_kit::call( | |
| 276 | 277 | &self.work, | |
| 277 | − | "get_issue", | |
| 278 | − | &ViewArgs { repo: repo.clone(), number, viewer: Some(actor.clone()), after_seq: 0 }, | |
| 278 | + | "get_work_item", | |
| 279 | + | &WorkItemViewArgs { workspace: workspace.to_owned(), number, viewer: Some(actor.clone()) }, | |
| 279 | 280 | ) | |
| 280 | 281 | .await?; | |
| 281 | − | Ok(found.into_result().ok().map(|detail| detail.issue)) | |
| 282 | + | Ok(found.into_result().ok().map(|detail| detail.item)) | |
| 282 | 283 | } | |
| 283 | 284 | ||
| 285 | + | /// Comments on pull request `number` of `repo`. | |
| 284 | 286 | pub(crate) async fn comment(&self, actor: &User, repo: &RepoPath, number: u32, body: String) -> Result<()> { | |
| 287 | + | self.comment_on(actor, Some(repo), "", number, body).await | |
| 288 | + | } | |
| 289 | + | ||
| 290 | + | /// Comments on a thread: with `repo`, its pull request `number`; | |
| 291 | + | /// without, the work item `number` of `workspace`. | |
| 292 | + | pub(crate) async fn comment_on(&self, actor: &User, repo: Option<&RepoPath>, workspace: &str, number: u32, body: String) -> Result<()> { | |
| 285 | 293 | let _: Outcome<Value> = g1t_kit::call( | |
| 286 | 294 | &self.work, | |
| 287 | 295 | "add_comment", | |
| 288 | 296 | &AddCommentArgs { | |
| 289 | 297 | actor: actor.clone(), | |
| 290 | − | repo: repo.clone(), | |
| 298 | + | repo: repo.cloned(), | |
| 299 | + | workspace: workspace.to_owned(), | |
| 291 | 300 | number, | |
| 292 | 301 | body, | |
| 293 | 302 | path: None, | |
| ⋯ | |||
| 363 | 372 | severity: advisory.severity.as_str().to_owned(), | |
| 364 | 373 | fixed_version: advisory.fixed.clone(), | |
| 365 | 374 | status: VulnStatus::Open, | |
| 366 | − | issue: None, | |
| 375 | + | item: None, | |
| 367 | 376 | found_at: String::new(), | |
| 368 | 377 | fixed_at: None, | |
| 369 | 378 | state: AlertState::Open, | |
| ⋯ | |||
| 443 | 452 | status: "open".into(), | |
| 444 | 453 | found_at: "2026-10-04T00:00:00Z".into(), | |
| 445 | 454 | fixed_at: None, | |
| 446 | − | number: None, | |
| 455 | + | item: None, | |
| 447 | 456 | dismiss_reason: None, | |
| 448 | 457 | dismiss_comment: None, | |
| 449 | 458 | dismissed_by: None, | |
| 643 | 643 | self.store.rename_namespace(&renamed.stale_slugs(&renamed.to), &renamed.to).await?; | |
| 644 | 644 | } | |
| 645 | 645 | } | |
| 646 | + | // Issues became work items: what was kept by their old numbers | |
| 647 | + | // names the work items now. | |
| 648 | + | "work.migrated" => { | |
| 649 | + | if let Ok(moved) = serde_json::from_value::<g1t_contracts::events::WorkMigrated>(event.data.clone()) { | |
| 650 | + | self.store.work_migrated(&moved.items).await?; | |
| 651 | + | } | |
| 652 | + | } | |
| 646 | 653 | _ => {} | |
| 647 | 654 | } | |
| 648 | 655 | Ok(()) |
| 246 | 246 | body.push('\n'); | |
| 247 | 247 | } | |
| 248 | 248 | body.push_str(&format!( | |
| 249 | − | "\nThis pull request lands through this branch's required checks like any other. If an update breaks them, g1t closes it and puts g1t on an issue to make the code changes it needs.\n\n{COMMANDS}\n\n---\n_Opened by g1t's version updates, as `{file}` asks{}._", | |
| 249 | + | "\nThis pull request lands through this branch's required checks like any other. If an update breaks them, g1t closes it and puts g1t on a work item to make the code changes it needs.\n\n{COMMANDS}\n\n---\n_Opened by g1t's version updates, as `{file}` asks{}._", | |
| 250 | 250 | entry.name.as_deref().map(|name| format!(" for {name}")).unwrap_or_default() | |
| 251 | 251 | )); | |
| 252 | 252 | body |
| 282 | 282 | if still_needed(&fixes) { | |
| 283 | 283 | continue; | |
| 284 | 284 | } | |
| 285 | − | self.store.set_update(&row, UpdateState::Superseded, row.pull(), row.issue(), None).await?; | |
| 285 | + | self.store.set_update(&row, UpdateState::Superseded, row.pull(), row.item(), None).await?; | |
| 286 | 286 | let Some(number) = row.pull() else { continue }; | |
| 287 | 287 | let packages = [row.package.clone()]; | |
| 288 | 288 | match self.retire(repo, number, |base| resolved_text(base, &packages, &fixes)).await? { | |
| 289 | − | Retired::Merged => self.store.set_update(&row, UpdateState::Merged, Some(number), row.issue(), None).await?, | |
| 289 | + | Retired::Merged => self.store.set_update(&row, UpdateState::Merged, Some(number), row.item(), None).await?, | |
| 290 | 290 | Retired::Closed => { | |
| 291 | 291 | let activity: Vec<Activity> = | |
| 292 | 292 | fixes.iter().map(|alert| Activity { alert_id: &alert.id, action: "update_superseded", actor: Some(system), reason: None, comment: None, number: Some(number) }).collect(); | |
| ⋯ | |||
| 343 | 343 | if let Some(update) = self.store.update(&repo.repo_id, osv, &dependency.name).await? | |
| 344 | 344 | && (update.state().in_progress() && (update.pull() == row.pull() || update.branch.as_deref() == Some(row.branch.as_str()))) | |
| 345 | 345 | { | |
| 346 | − | self.store.set_update(&update, state, update.pull(), update.issue(), None).await?; | |
| 346 | + | self.store.set_update(&update, state, update.pull(), update.item(), None).await?; | |
| 347 | 347 | } | |
| 348 | 348 | } | |
| 349 | 349 | } | |
| ⋯ | |||
| 426 | 426 | status: status.into(), | |
| 427 | 427 | found_at: "2026-10-04T00:00:00Z".into(), | |
| 428 | 428 | fixed_at: None, | |
| 429 | − | number: None, | |
| 429 | + | item: None, | |
| 430 | 430 | dismiss_reason: None, | |
| 431 | 431 | dismiss_comment: None, | |
| 432 | 432 | dismissed_by: None, | |
| 8 | 8 | //! (`User::system`). It lands through the branch's required checks like | |
| 9 | 9 | //! any other. An older one for the same package is closed as superseded. | |
| 10 | 10 | //! 3. When its checks fail because code has to change, or the sandbox | |
| 11 | − | //! never pushed, the pull request is closed and an issue is opened for | |
| 11 | + | //! never pushed, the pull request is closed and a work item is opened for | |
| 12 | 12 | //! g1t to work on, started by g1t. That is the only time an agent is | |
| 13 | 13 | //! used. | |
| 14 | 14 | //! 4. Once none of the alerts it fixes is open (fixed on the default | |
| ⋯ | |||
| 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::{IssueActionArgs, IssueReason, OpenPullArgs, Pull, PullActionArgs, PullDetail, PullStatus, Runtime, ViewArgs}; | |
| 25 | + | use g1t_contracts::work::{ | |
| 26 | + | CloseReason, CloseWorkItemArgs, OpenPullArgs, Pull, PullActionArgs, PullDetail, PullStatus, Runtime, ViewArgs, WorkItemActionArgs, | |
| 27 | + | }; | |
| 26 | 28 | use g1t_contracts::{Outcome, User}; | |
| 27 | 29 | use g1t_kit::now_ms; | |
| 28 | 30 | use g1t_scan::osv::{self, Severity}; | |
| ⋯ | |||
| 105 | 107 | body.push_str(&format!( | |
| 106 | 108 | "\nLockfiles changed: {}.\n\n\ | |
| 107 | 109 | Only the version changes. This pull request lands through this branch's required checks like any other. \ | |
| 108 | − | If they fail because code has to change, g1t closes it and puts g1t on an issue to make the change.\n\n\ | |
| 110 | + | If they fail because code has to change, g1t closes it and puts g1t on a work item to make the change.\n\n\ | |
| 109 | 111 | ---\n_Opened by g1t's security updates. Turn them off for this project on its Security page._", | |
| 110 | 112 | lockfiles.iter().map(|path| format!("`{path}`")).collect::<Vec<_>>().join(", ") | |
| 111 | 113 | )); | |
| ⋯ | |||
| 210 | 212 | body.push_str(&advisory_table(vulns)); | |
| 211 | 213 | body.push_str(&format!( | |
| 212 | 214 | "\nOnly the versions change. This pull request lands through this branch's required checks like any other. \ | |
| 213 | − | If they fail because code has to change, g1t closes it and puts g1t on an issue to make the change.\n\n{}\n\n---\n\ | |
| 215 | + | If they fail because code has to change, g1t closes it and puts g1t on a work item to make the change.\n\n{}\n\n---\n\ | |
| 214 | 216 | _Opened by g1t's security updates, grouped as `{file}` asks. Turn them off for this project on its Security page._", | |
| 215 | 217 | pull_text::COMMANDS | |
| 216 | 218 | )); | |
| ⋯ | |||
| 245 | 247 | repo: Self::path_of(repo), | |
| 246 | 248 | number, | |
| 247 | 249 | summary: String::new(), | |
| 248 | − | keep_issue_open: false, | |
| 250 | + | keep_item_open: false, | |
| 249 | 251 | ignore_checks: false, | |
| 250 | 252 | bypass_rules: false, | |
| 251 | 253 | merge_method: None, | |
| ⋯ | |||
| 263 | 265 | Ok(matches!(closed, Outcome::Ok(_))) | |
| 264 | 266 | } | |
| 265 | 267 | ||
| 266 | − | /// Closes an issue g1t opened for an update before it stopped opening | |
| 267 | − | /// them, saying why. One already closed is left as it is. | |
| 268 | − | pub(crate) async fn close_issue_with(&self, repo: &RepoRow, number: u32, why: String) -> Result<()> { | |
| 268 | + | /// The slug of the workspace a repository is in. | |
| 269 | + | pub(crate) fn workspace_of(repo: &RepoRow) -> String { | |
| 270 | + | repo.namespace.to_lowercase() | |
| 271 | + | } | |
| 272 | + | ||
| 273 | + | /// Closes a work item g1t opened for an update before it stopped | |
| 274 | + | /// opening them, saying why. One already closed is left as it is. | |
| 275 | + | pub(crate) async fn close_item_with(&self, repo: &RepoRow, number: u32, why: String) -> Result<()> { | |
| 269 | 276 | let system = User::system(&repo.namespace); | |
| 270 | − | let path = Self::path_of(repo); | |
| 271 | − | let closed: Outcome<Value> = g1t_kit::call( | |
| 272 | − | &self.work, | |
| 273 | − | "close_issue", | |
| 274 | − | &IssueActionArgs { actor: system.clone(), repo: path.clone(), number, reason: Some(IssueReason::NotPlanned) }, | |
| 275 | − | ) | |
| 276 | − | .await?; | |
| 277 | + | let workspace = Self::workspace_of(repo); | |
| 278 | + | let item = WorkItemActionArgs { actor: system.clone(), workspace: workspace.clone(), number, reason: Some(CloseReason::NotPlanned) }; | |
| 279 | + | let closed: Outcome<Value> = | |
| 280 | + | g1t_kit::call(&self.work, "close_work_item", &CloseWorkItemArgs { item, repo: None, commit: None }).await?; | |
| 277 | 281 | if matches!(closed, Outcome::Ok(_)) { | |
| 278 | − | self.comment(&system, &path, number, why).await?; | |
| 282 | + | self.comment_on(&system, None, &workspace, number, why).await?; | |
| 279 | 283 | } | |
| 280 | 284 | Ok(()) | |
| 281 | 285 | } | |
| ⋯ | |||
| 350 | 354 | } | |
| 351 | 355 | } | |
| 352 | 356 | None => { | |
| 353 | − | // An upgrade issue from before security updates, still | |
| 354 | − | // open, is left to the agent on it. | |
| 357 | + | // An upgrade work item from before security updates, | |
| 358 | + | // still open, is left to the agent on it. | |
| 355 | 359 | if let Some(existing) = self.store.upgrade(&repo.repo_id, &ecosystem, &package).await? | |
| 356 | 360 | && self | |
| 357 | − | .issue(&User::system(&repo.namespace), &Self::path_of(repo), existing.number as u32) | |
| 361 | + | .item(&User::system(&repo.namespace), &Self::workspace_of(repo), existing.item as u32) | |
| 358 | 362 | .await? | |
| 359 | − | .is_some_and(|issue| issue.state == g1t_contracts::work::State::Open) | |
| 363 | + | .is_some_and(|item| item.state == g1t_contracts::work::State::Open) | |
| 360 | 364 | { | |
| 361 | 365 | continue; | |
| 362 | 366 | } | |
| ⋯ | |||
| 539 | 543 | &OpenPullArgs { | |
| 540 | 544 | actor: system.clone(), | |
| 541 | 545 | repo: Self::path_of(&repo), | |
| 542 | − | issue: None, | |
| 546 | + | item: None, | |
| 543 | 547 | title: prefixed(&prefix, pull_title(&row.package, &row.target)), | |
| 544 | 548 | body: pull_text(&row.ecosystem, &row.package, &row.target, &vulns), | |
| 545 | 549 | branch: Some(branch.to_owned()), | |
| ⋯ | |||
| 656 | 660 | self.store.set_update(&row, UpdateState::Failed, row.pull(), None, Some(&why)).await?; | |
| 657 | 661 | self.note(&row, "update_failed", Some(g1t_contracts::system::USERNAME), None, Some(&why)).await?; | |
| 658 | 662 | } | |
| 659 | − | // Left from when g1t opened an issue for an upgrade it could not | |
| 660 | − | // make: the issue closes, and the update fails as one would now. | |
| 663 | + | // Left from when g1t opened a work item for an upgrade it could not | |
| 664 | + | // make: the work item closes, and the update fails as one would now. | |
| 661 | 665 | for row in self.store.needs_code_updates(10).await? { | |
| 662 | 666 | let why = cannot_raise(&row.package, &row.target); | |
| 663 | − | if let (Some(repo), Some(number)) = (self.store.repo(&row.repo_id).await?, row.issue()) { | |
| 667 | + | if let (Some(repo), Some(number)) = (self.store.repo(&row.repo_id).await?, row.item()) { | |
| 664 | 668 | let path = Self::path_of(&repo); | |
| 665 | 669 | let alerts = format!("/{}/{}/security/vulnerabilities", path.namespace, path.name); | |
| 666 | − | self.close_issue_with(&repo, number, format!( | |
| 667 | − | "g1t no longer opens issues for upgrades it cannot make on its own. This one is on [its alert]({alerts}), \ | |
| 670 | + | self.close_item_with(&repo, number, format!( | |
| 671 | + | "g1t no longer opens work items for upgrades it cannot make on its own. This one is on [its alert]({alerts}), \ | |
| 668 | 672 | which says why and is tried again later." | |
| 669 | 673 | )) | |
| 670 | 674 | .await?; | |
| ⋯ | |||
| 742 | 746 | status: "open".into(), | |
| 743 | 747 | found_at: "2026-10-04T00:00:00Z".into(), | |
| 744 | 748 | fixed_at: None, | |
| 745 | − | number: None, | |
| 749 | + | item: None, | |
| 746 | 750 | dismiss_reason: None, | |
| 747 | 751 | dismiss_comment: None, | |
| 748 | 752 | dismissed_by: None, | |
| 113 | 113 | } | |
| 114 | 114 | ||
| 115 | 115 | /// Secrets with how many places each was found. | |
| 116 | + | /// Rewrites the work items kept by the old issue numbers of their | |
| 117 | + | /// repositories to their numbers in the workspace, `?1` a page of | |
| 118 | + | /// `work.migrated`'s items as JSON. Each new number is above every old one | |
| 119 | + | /// of its workspace, so a page heard twice changes nothing more. | |
| 120 | + | pub const WORK_MIGRATED: [&str; 6] = [ | |
| 121 | + | "UPDATE updates SET item = m.number FROM moved_items m WHERE updates.repo_id = m.repo_id AND updates.item = m.legacy", | |
| 122 | + | "UPDATE update_pulls SET item = m.number FROM moved_items m WHERE update_pulls.repo_id = m.repo_id AND update_pulls.item = m.legacy", | |
| 123 | + | "UPDATE upgrades SET item = m.number FROM moved_items m WHERE upgrades.repo_id = m.repo_id AND upgrades.item = m.legacy", | |
| 124 | + | "UPDATE code_alerts SET item = m.number FROM moved_items m WHERE code_alerts.repo_id = m.repo_id AND code_alerts.item = m.legacy", | |
| 125 | + | "UPDATE alert_fixes SET item = m.number FROM moved_items m WHERE alert_fixes.repo_id = m.repo_id AND alert_fixes.item = m.legacy", | |
| 126 | + | // Only an update that needed code changes named a work item; the | |
| 127 | + | // other steps name pull requests, which kept their numbers. | |
| 128 | + | "UPDATE alert_activity SET number = m.number FROM moved_items m | |
| 129 | + | WHERE alert_activity.repo_id = m.repo_id AND alert_activity.number = m.legacy AND alert_activity.action = 'update_needs_code'", | |
| 130 | + | ]; | |
| 131 | + | ||
| 132 | + | /// `work.migrated`'s items (`?1`), as the rows [`WORK_MIGRATED`] reads. | |
| 133 | + | const MOVED_ITEMS: &str = "WITH moved_items AS ( | |
| 134 | + | SELECT json_extract(value, '$.repoId') AS repo_id, json_extract(value, '$.legacyNumber') AS legacy, | |
| 135 | + | json_extract(value, '$.number') AS number | |
| 136 | + | FROM json_each(?1))"; | |
| 137 | + | ||
| 138 | + | /// One of [`WORK_MIGRATED`], ready to run. | |
| 139 | + | pub fn work_migrated_sql(statement: &str) -> String { | |
| 140 | + | format!("{MOVED_ITEMS} | |
| 141 | + | {statement}") | |
| 142 | + | } | |
| 143 | + | ||
| 116 | 144 | const SECRET_SELECT: &str = | |
| 117 | 145 | "SELECT s.*, (SELECT count(*) FROM secret_locations l WHERE l.secret_id = s.id) AS location_count FROM secrets s"; | |
| 118 | 146 | ||
| ⋯ | |||
| 174 | 202 | pub status: String, | |
| 175 | 203 | pub found_at: String, | |
| 176 | 204 | pub fixed_at: Option<String>, | |
| 177 | − | pub number: Option<i64>, | |
| 205 | + | pub item: Option<i64>, | |
| 178 | 206 | #[serde(default)] | |
| 179 | 207 | pub dismiss_reason: Option<String>, | |
| 180 | 208 | #[serde(default)] | |
| ⋯ | |||
| 202 | 230 | severity: row.severity, | |
| 203 | 231 | fixed_version: row.fixed_version, | |
| 204 | 232 | status, | |
| 205 | − | issue: row.number.map(|number| number as u32), | |
| 233 | + | item: row.item.map(|number| number as u32), | |
| 206 | 234 | found_at: row.found_at, | |
| 207 | 235 | fixed_at: row.fixed_at, | |
| 208 | 236 | state: status.state(), | |
| ⋯ | |||
| 225 | 253 | pub state: String, | |
| 226 | 254 | pub branch: Option<String>, | |
| 227 | 255 | pub pull: Option<i64>, | |
| 228 | − | pub issue: Option<i64>, | |
| 256 | + | pub item: Option<i64>, | |
| 229 | 257 | pub error: Option<String>, | |
| 230 | 258 | pub updated_at: String, | |
| 231 | 259 | } | |
| ⋯ | |||
| 239 | 267 | self.pull.map(|n| n.max(0) as u32) | |
| 240 | 268 | } | |
| 241 | 269 | ||
| 242 | − | pub fn issue(&self) -> Option<u32> { | |
| 243 | − | self.issue.map(|n| n.max(0) as u32) | |
| 270 | + | pub fn item(&self) -> Option<u32> { | |
| 271 | + | self.item.map(|n| n.max(0) as u32) | |
| 244 | 272 | } | |
| 245 | 273 | ||
| 246 | 274 | pub fn to_contract(&self) -> SecurityUpdate { | |
| ⋯ | |||
| 249 | 277 | target: self.target.clone(), | |
| 250 | 278 | branch: self.branch.clone(), | |
| 251 | 279 | pull: self.pull(), | |
| 252 | − | issue: self.issue(), | |
| 280 | + | item: self.item(), | |
| 253 | 281 | error: self.error.clone(), | |
| 254 | 282 | updated_at: self.updated_at.clone(), | |
| 255 | 283 | } | |
| ⋯ | |||
| 308 | 336 | ||
| 309 | 337 | #[derive(Deserialize)] | |
| 310 | 338 | pub struct UpgradeRow { | |
| 311 | − | pub number: i64, | |
| 339 | + | pub item: i64, | |
| 312 | 340 | } | |
| 313 | 341 | ||
| 314 | 342 | #[derive(Deserialize)] | |
| ⋯ | |||
| 348 | 376 | ||
| 349 | 377 | const VULN_COLUMNS: &str = "v.id, v.repo_id, v.ecosystem, v.package, v.version, v.manifest, v.osv_id, v.advisory, | |
| 350 | 378 | v.summary, v.severity, v.fixed_version, v.status, v.found_at, v.fixed_at, v.dismiss_reason, v.dismiss_comment, | |
| 351 | − | v.dismissed_by, v.dismissed_at, COALESCE(n.issue, u.number) AS number"; | |
| 379 | + | v.dismissed_by, v.dismissed_at, COALESCE(n.item, u.item) AS item"; | |
| 352 | 380 | ||
| 353 | − | /// The issue an upgrade has: the security update's, when it went to | |
| 381 | + | /// The work item an upgrade has: the security update's, when it went to | |
| 354 | 382 | /// g1t, or one opened before security updates. | |
| 355 | 383 | const VULN_JOINS: &str = "LEFT JOIN upgrades u ON u.repo_id = v.repo_id AND u.ecosystem = v.ecosystem AND u.package = v.package | |
| 356 | 384 | LEFT JOIN updates n ON n.repo_id = v.repo_id AND n.ecosystem = v.ecosystem AND n.package = v.package"; | |
| ⋯ | |||
| 420 | 448 | Ok(()) | |
| 421 | 449 | } | |
| 422 | 450 | ||
| 451 | + | /// `work.migrated`: what was kept by old issue numbers names the work | |
| 452 | + | /// items they became. | |
| 453 | + | pub async fn work_migrated(&self, items: &[g1t_contracts::work::MigratedItem]) -> Result<()> { | |
| 454 | + | if items.is_empty() { | |
| 455 | + | return Ok(()); | |
| 456 | + | } | |
| 457 | + | let page = serde_json::to_string(items)?; | |
| 458 | + | let mut batch = Vec::with_capacity(WORK_MIGRATED.len()); | |
| 459 | + | for sql in WORK_MIGRATED { | |
| 460 | + | batch.push(self.db.prepare(work_migrated_sql(sql)).bind(&[page.as_str().into()])?); | |
| 461 | + | } | |
| 462 | + | self.db.batch(batch).await?; | |
| 463 | + | Ok(()) | |
| 464 | + | } | |
| 465 | + | ||
| 423 | 466 | /// Records that a repository's daily dependency read was skipped, and | |
| 424 | 467 | /// why, so the sweep moves on to others until the next day. | |
| 425 | 468 | pub async fn skip_dependencies(&self, repo_id: &str, why: &str) -> Result<()> { | |
| ⋯ | |||
| 984 | 1027 | } | |
| 985 | 1028 | ||
| 986 | 1029 | /// Updates asked of a sandbox before `before` that never pushed. | |
| 987 | − | /// Updates left in `needs_code`, which g1t opened an issue for before | |
| 1030 | + | /// Updates left in `needs_code`, which g1t opened a work item for before | |
| 988 | 1031 | /// it stopped doing that. | |
| 989 | 1032 | pub async fn needs_code_updates(&self, limit: u32) -> Result<Vec<UpdateRow>> { | |
| 990 | 1033 | self.db | |
| ⋯ | |||
| 1014 | 1057 | "INSERT INTO updates (repo_id, ecosystem, package, target, state, branch, requested_at, updated_at) | |
| 1015 | 1058 | VALUES (?1, ?2, ?3, ?4, 'requested', ?5, ?6, ?6) | |
| 1016 | 1059 | ON CONFLICT (repo_id, ecosystem, package) DO UPDATE SET | |
| 1017 | − | target = ?4, state = 'requested', branch = ?5, error = NULL, issue = NULL, | |
| 1060 | + | target = ?4, state = 'requested', branch = ?5, error = NULL, item = NULL, | |
| 1018 | 1061 | pull = CASE WHEN updates.state = 'open' THEN updates.pull ELSE NULL END, | |
| 1019 | 1062 | requested_at = ?6, updated_at = ?6", | |
| 1020 | 1063 | ) | |
| ⋯ | |||
| 1029 | 1072 | row: &UpdateRow, | |
| 1030 | 1073 | state: UpdateState, | |
| 1031 | 1074 | pull: Option<u32>, | |
| 1032 | − | issue: Option<u32>, | |
| 1075 | + | item: Option<u32>, | |
| 1033 | 1076 | error: Option<&str>, | |
| 1034 | 1077 | ) -> Result<()> { | |
| 1035 | 1078 | self.db | |
| 1036 | 1079 | .prepare( | |
| 1037 | − | "UPDATE updates SET state = ?, pull = ?, issue = ?, error = ?, updated_at = ? | |
| 1080 | + | "UPDATE updates SET state = ?, pull = ?, item = ?, error = ?, updated_at = ? | |
| 1038 | 1081 | WHERE repo_id = ? AND ecosystem = ? AND package = ?", | |
| 1039 | 1082 | ) | |
| 1040 | 1083 | .bind(&[ | |
| 1041 | 1084 | state.as_str().into(), | |
| 1042 | 1085 | pull.map_or(JsValue::NULL, JsValue::from), | |
| 1043 | − | issue.map_or(JsValue::NULL, JsValue::from), | |
| 1086 | + | item.map_or(JsValue::NULL, JsValue::from), | |
| 1044 | 1087 | optional(error), | |
| 1045 | 1088 | now().into(), | |
| 1046 | 1089 | row.repo_id.as_str().into(), | |
| ⋯ | |||
| 1108 | 1151 | ||
| 1109 | 1152 | // --- Upgrades ----------------------------------------------------------- | |
| 1110 | 1153 | ||
| 1111 | − | /// The issue opened for a package before security updates, if any. | |
| 1154 | + | /// The work item opened for a package before security updates, if any. | |
| 1112 | 1155 | pub async fn upgrade(&self, repo_id: &str, ecosystem: &str, package: &str) -> Result<Option<UpgradeRow>> { | |
| 1113 | 1156 | self.db | |
| 1114 | − | .prepare("SELECT number FROM upgrades WHERE repo_id = ? AND ecosystem = ? AND package = ?") | |
| 1157 | + | .prepare("SELECT item FROM upgrades WHERE repo_id = ? AND ecosystem = ? AND package = ?") | |
| 1115 | 1158 | .bind(&[repo_id.into(), ecosystem.into(), package.into()])? | |
| 1116 | 1159 | .first::<UpgradeRow>(None) | |
| 1117 | 1160 | .await | |
| ⋯ | |||
| 1198 | 1241 | mod tests { | |
| 1199 | 1242 | use super::*; | |
| 1200 | 1243 | ||
| 1244 | + | /// The migrations, in order, from `migrations/`. | |
| 1245 | + | fn migrations() -> Vec<(String, String)> { | |
| 1246 | + | let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("migrations"); | |
| 1247 | + | let mut names: Vec<String> = | |
| 1248 | + | std::fs::read_dir(&dir).unwrap().map(|entry| entry.unwrap().file_name().into_string().unwrap()).collect(); | |
| 1249 | + | names.sort(); | |
| 1250 | + | names.into_iter().map(|name| (name.clone(), std::fs::read_to_string(dir.join(&name)).unwrap())).collect() | |
| 1251 | + | } | |
| 1252 | + | ||
| 1253 | + | #[test] | |
| 1254 | + | fn old_issue_numbers_become_their_work_items_numbers() { | |
| 1255 | + | let db = rusqlite::Connection::open_in_memory().unwrap(); | |
| 1256 | + | let (before, after): (Vec<_>, Vec<_>) = migrations().into_iter().partition(|(name, _)| name.as_str() < "0007"); | |
| 1257 | + | for (name, sql) in &before { | |
| 1258 | + | db.execute_batch(sql).unwrap_or_else(|error| panic!("{name}: {error}")); | |
| 1259 | + | } | |
| 1260 | + | db.execute_batch( | |
| 1261 | + | "INSERT INTO updates (repo_id, ecosystem, package, target, state, pull, issue, requested_at, updated_at) | |
| 1262 | + | VALUES ('rep_1', 'npm', 'a', '2', 'needs_code', NULL, 5, '', ''), ('rep_2', 'npm', 'a', '2', 'needs_code', NULL, 5, '', ''); | |
| 1263 | + | INSERT INTO upgrades (repo_id, ecosystem, package, number, target, opened_at) VALUES ('rep_1', 'npm', 'b', 3, '2', ''); | |
| 1264 | + | INSERT INTO alert_activity (id, repo_id, alert_id, action, number, at) | |
| 1265 | + | VALUES ('a1', 'rep_1', 'vul_1', 'update_needs_code', 5, ''), ('a2', 'rep_1', 'vul_1', 'update_opened', 5, '');", | |
| 1266 | + | ) | |
| 1267 | + | .unwrap(); | |
| 1268 | + | for (name, sql) in &after { | |
| 1269 | + | db.execute_batch(sql).unwrap_or_else(|error| panic!("{name}: {error}")); | |
| 1270 | + | } | |
| 1271 | + | let page = serde_json::json!([ | |
| 1272 | + | { "repoId": "rep_1", "legacyNumber": 5, "workspace": "acme", "number": 41, "itemId": "iss_5" }, | |
| 1273 | + | { "repoId": "rep_1", "legacyNumber": 3, "workspace": "acme", "number": 40, "itemId": "iss_3" }, | |
| 1274 | + | ]) | |
| 1275 | + | .to_string(); | |
| 1276 | + | // Heard twice, as a queue may deliver it. | |
| 1277 | + | for _ in 0..2 { | |
| 1278 | + | for statement in WORK_MIGRATED { | |
| 1279 | + | db.execute(&work_migrated_sql(statement), [&page]).unwrap(); | |
| 1280 | + | } | |
| 1281 | + | } | |
| 1282 | + | let item = |sql: &str| db.query_row(sql, [], |row| row.get::<_, Option<i64>>(0)).unwrap(); | |
| 1283 | + | assert_eq!(item("SELECT item FROM updates WHERE repo_id = 'rep_1'"), Some(41)); | |
| 1284 | + | // Another repository's number 5 was another issue. | |
| 1285 | + | assert_eq!(item("SELECT item FROM updates WHERE repo_id = 'rep_2'"), Some(5)); | |
| 1286 | + | assert_eq!(item("SELECT item FROM upgrades WHERE repo_id = 'rep_1'"), Some(40)); | |
| 1287 | + | assert_eq!(item("SELECT number FROM alert_activity WHERE id = 'a1'"), Some(41)); | |
| 1288 | + | // A pull request kept its number. | |
| 1289 | + | assert_eq!(item("SELECT number FROM alert_activity WHERE id = 'a2'"), Some(5)); | |
| 1290 | + | } | |
| 1291 | + | ||
| 1201 | 1292 | #[test] | |
| 1202 | 1293 | fn only_real_secrets_in_history_are_open() { | |
| 1203 | 1294 | let counts = secret_counts( | |
| 204 | 204 | pub dismiss_comment: Option<String>, | |
| 205 | 205 | pub dismissed_by: Option<String>, | |
| 206 | 206 | pub dismissed_at: Option<String>, | |
| 207 | − | pub issue: Option<i64>, | |
| 207 | + | pub item: Option<i64>, | |
| 208 | 208 | } | |
| 209 | 209 | ||
| 210 | 210 | fn small(value: Option<i64>) -> Option<u32> { | |
| ⋯ | |||
| 247 | 247 | dismissed_reason: row.dismiss_reason.as_deref().and_then(DismissReason::parse).filter(|_| dismissed), | |
| 248 | 248 | dismissed_comment: row.dismiss_comment.filter(|_| dismissed), | |
| 249 | 249 | dismissed_at: row.dismissed_at.filter(|_| dismissed), | |
| 250 | − | issue: small(row.issue), | |
| 250 | + | item: small(row.item), | |
| 251 | 251 | } | |
| 252 | 252 | } | |
| 253 | 253 | } | |
| 371 | 371 | let done: Result<Outcome<Value>> = match comment_plan(our_comment(comments), &body, changes) { | |
| 372 | 372 | CommentPlan::Leave => return, | |
| 373 | 373 | CommentPlan::Edit(comment_id) => { | |
| 374 | − | g1t_kit::call(&self.work, "edit_comment", &CommentActionArgs { actor, repo: path, comment_id, body }).await | |
| 374 | + | g1t_kit::call(&self.work, "edit_comment", &CommentActionArgs { actor, repo: Some(path), workspace: String::new(), comment_id, body }).await | |
| 375 | 375 | } | |
| 376 | 376 | CommentPlan::Add => { | |
| 377 | − | g1t_kit::call(&self.work, "add_comment", &AddCommentArgs { actor, repo: path, number, body, path: None, line: None, verdict: None }) | |
| 378 | − | .await | |
| 377 | + | g1t_kit::call(&self.work, "add_comment", &AddCommentArgs { | |
| 378 | + | actor, | |
| 379 | + | repo: Some(path), | |
| 380 | + | workspace: String::new(), | |
| 381 | + | number, | |
| 382 | + | body, | |
| 383 | + | path: None, | |
| 384 | + | line: None, | |
| 385 | + | verdict: None, | |
| 386 | + | }, | |
| 387 | + | ) | |
| 388 | + | .await | |
| 379 | 389 | } | |
| 380 | 390 | }; | |
| 381 | 391 | match done { |
| 42 | 42 | pub reviewers: String, | |
| 43 | 43 | pub state: String, | |
| 44 | 44 | pub pull: Option<i64>, | |
| 45 | − | /// The issue g1t opened for it, from before it stopped doing that. | |
| 45 | + | /// The work item g1t opened for it, from before it stopped doing that. | |
| 46 | 46 | #[serde(default)] | |
| 47 | − | pub issue: Option<i64>, | |
| 47 | + | pub item: Option<i64>, | |
| 48 | 48 | pub head: Option<String>, | |
| 49 | 49 | pub merge_by: Option<String>, | |
| 50 | 50 | pub error: Option<String>, | |
| ⋯ | |||
| 60 | 60 | self.pull.map(|n| n.max(0) as u32) | |
| 61 | 61 | } | |
| 62 | 62 | ||
| 63 | − | pub fn issue(&self) -> Option<u32> { | |
| 64 | − | self.issue.map(|n| n.max(0) as u32) | |
| 63 | + | pub fn item(&self) -> Option<u32> { | |
| 64 | + | self.item.map(|n| n.max(0) as u32) | |
| 65 | 65 | } | |
| 66 | 66 | ||
| 67 | 67 | pub fn dependencies(&self) -> Vec<UpdatedDependency> { | |
| ⋯ | |||
| 259 | 259 | } | |
| 260 | 260 | ||
| 261 | 261 | /// Update pull requests asked of a sandbox before `before` that never pushed. | |
| 262 | − | /// Version updates left in `needs_code`, which g1t opened an issue for | |
| 262 | + | /// Version updates left in `needs_code`, which g1t opened a work item for | |
| 263 | 263 | /// before it stopped doing that. | |
| 264 | 264 | pub async fn needs_code_update_pulls(&self, limit: u32) -> Result<Vec<PullRow>> { | |
| 265 | 265 | self.db | |
| ⋯ | |||
| 311 | 311 | Ok(id) | |
| 312 | 312 | } | |
| 313 | 313 | ||
| 314 | − | pub async fn set_update_pull(&self, id: &str, state: UpdateState, pull: Option<u32>, issue: Option<u32>, error: Option<&str>) -> Result<()> { | |
| 314 | + | pub async fn set_update_pull(&self, id: &str, state: UpdateState, pull: Option<u32>, item: Option<u32>, error: Option<&str>) -> Result<()> { | |
| 315 | 315 | self.db | |
| 316 | − | .prepare("UPDATE update_pulls SET state = ?, pull = ?, issue = ?, error = ?, updated_at = ? WHERE id = ?") | |
| 316 | + | .prepare("UPDATE update_pulls SET state = ?, pull = ?, item = ?, error = ?, updated_at = ? WHERE id = ?") | |
| 317 | 317 | .bind(&[ | |
| 318 | 318 | state.as_str().into(), | |
| 319 | 319 | pull.map_or(JsValue::NULL, JsValue::from), | |
| 320 | − | issue.map_or(JsValue::NULL, JsValue::from), | |
| 320 | + | item.map_or(JsValue::NULL, JsValue::from), | |
| 321 | 321 | optional(error), | |
| 322 | 322 | now().into(), | |
| 323 | 323 | id.into(), | |
| 16 | 16 | //! asked as the file says. An older one for the same dependency or group | |
| 17 | 17 | //! is closed as superseded. | |
| 18 | 18 | //! 4. The sweep watches open ones: a failing required check closes it and | |
| 19 | − | //! puts g1t on an issue to make the code changes, as security updates | |
| 19 | + | //! puts g1t on a work item to make the code changes, as security updates | |
| 20 | 20 | //! do; one asked to merge (`@g1t merge`) merges once its checks pass; | |
| 21 | 21 | //! one in conflict is made again from its base (`rebase-strategy`). | |
| 22 | 22 | //! 5. `@g1t` comments on these pull requests are commands | |
| ⋯ | |||
| 399 | 399 | let error = "The update could not be made in a sandbox: its packages may need code changes to move, or a registry could not be reached."; | |
| 400 | 400 | self.store.set_update_pull(&row.id, UpdateState::Failed, row.pull(), None, Some(error)).await?; | |
| 401 | 401 | } | |
| 402 | − | // Left from when g1t opened an issue for an update whose checks | |
| 403 | − | // failed: the issue closes, and the next run opens the update again | |
| 402 | + | // Left from when g1t opened a work item for an update whose checks | |
| 403 | + | // failed: the work item closes, and the next run opens the update again | |
| 404 | 404 | // as a pull request that stays open with its failing checks. | |
| 405 | 405 | for row in self.store.needs_code_update_pulls(10).await? { | |
| 406 | − | if let (Some(repo), Some(number)) = (self.store.repo(&row.repo_id).await?, row.issue()) { | |
| 407 | − | let why = "g1t no longer opens issues for updates whose checks fail. The entry's next run opens this update again \ | |
| 406 | + | if let (Some(repo), Some(number)) = (self.store.repo(&row.repo_id).await?, row.item()) { | |
| 407 | + | let why = "g1t no longer opens work items for updates whose checks fail. The entry's next run opens this update again \ | |
| 408 | 408 | as a pull request, which stays open with its checks for you to decide on."; | |
| 409 | − | self.close_issue_with(&repo, number, why.to_owned()).await?; | |
| 409 | + | self.close_item_with(&repo, number, why.to_owned()).await?; | |
| 410 | 410 | } | |
| 411 | 411 | let error = "Raising the versions failed this branch's required checks."; | |
| 412 | 412 | self.store.set_update_pull(&row.id, UpdateState::Failed, row.pull(), None, Some(error)).await?; | |
| ⋯ | |||
| 931 | 931 | &OpenPullArgs { | |
| 932 | 932 | actor: system.clone(), | |
| 933 | 933 | repo: Self::repo_path(repo), | |
| 934 | − | issue: None, | |
| 934 | + | item: None, | |
| 935 | 935 | title: row.title.clone(), | |
| 936 | 936 | body: row.body.clone(), | |
| 937 | 937 | branch: Some(row.branch.clone()), | |
| ⋯ | |||
| 1073 | 1073 | repo: Self::repo_path(&repo), | |
| 1074 | 1074 | number, | |
| 1075 | 1075 | summary: String::new(), | |
| 1076 | − | keep_issue_open: false, | |
| 1076 | + | keep_item_open: false, | |
| 1077 | 1077 | ignore_checks: false, | |
| 1078 | 1078 | bypass_rules: false, | |
| 1079 | 1079 | merge_method: None, | |
| ⋯ | |||
| 1376 | 1376 | let merged: Outcome<Pull> = g1t_kit::call( | |
| 1377 | 1377 | &self.work, | |
| 1378 | 1378 | "merge_pull", | |
| 1379 | − | &PullActionArgs { actor: system.clone(), repo: path.clone(), number, summary: String::new(), keep_issue_open: false, ignore_checks: false, bypass_rules: false, merge_method: matches!(command, UpdateCommand::SquashAndMerge).then_some(g1t_contracts::rules::MergeMethod::Squash), commit_title: None, commit_message: None, expected_head: None, delete_branch: None, update_method: None }, | |
| 1379 | + | &PullActionArgs { actor: system.clone(), repo: path.clone(), number, summary: String::new(), keep_item_open: false, ignore_checks: false, bypass_rules: false, merge_method: matches!(command, UpdateCommand::SquashAndMerge).then_some(g1t_contracts::rules::MergeMethod::Squash), commit_title: None, commit_message: None, expected_head: None, delete_branch: None, update_method: None }, | |
| 1380 | 1380 | ) | |
| 1381 | 1381 | .await?; | |
| 1382 | 1382 | match merged { | |
| ⋯ | |||
| 1586 | 1586 | reviewers: "[]".into(), | |
| 1587 | 1587 | state: state.into(), | |
| 1588 | 1588 | pull: Some(3), | |
| 1589 | − | issue: None, | |
| 1589 | + | item: None, | |
| 1590 | 1590 | head: None, | |
| 1591 | 1591 | merge_by: None, | |
| 1592 | 1592 | error: None, | |
| ⋯ | |||
| 1636 | 1636 | let status = |state: &str| CommitStatus { context: "CI / push".into(), state: state.into(), description: None, target_url: None, updated_at: String::new(), source: None, check_run_id: None }; | |
| 1637 | 1637 | let detail = |required: Vec<RequiredCheck>, statuses: Vec<CommitStatus>| { | |
| 1638 | 1638 | let mut detail: PullDetail = serde_json::from_value(json!({ | |
| 1639 | − | "pull": {"id": "pul_1", "repoId": "rep_1", "number": 1, "issue": null, "title": "t", "body": null, "agent": "", "runtime": "external", | |
| 1639 | + | "pull": {"id": "pul_1", "repoId": "rep_1", "number": 1, "item": null, "title": "t", "body": null, "agent": "", "runtime": "external", | |
| 1640 | 1640 | "status": "open", "fork": null, "forkRepoId": null, "branch": "b", "headCommit": "c", "mergeBase": null, "mergedBy": null, | |
| 1641 | 1641 | "mergedAt": null, "supersededBy": null, "checkStatus": null, | |
| 1642 | 1642 | "author": {"id": "g1t", "username": "g1t", "kind": "system"}, "createdAt": "", "updatedAt": ""}, | |
| 1643 | − | "issue": null, "comments": [], "checks": null | |
| 1643 | + | "item": null, "comments": [], "checks": null | |
| 1644 | 1644 | })) | |
| 1645 | 1645 | .unwrap(); | |
| 1646 | 1646 | detail.required_checks = required; | |
Comments
Sign in to comment.