Skip to content

Commit

Merge update PRs close themselves: g1t closes its security and version updates once they are no longer needed, and deletes their branches

syntaqxcommitted Parentsaaf1ffb2bd3694Browse files
11 files+696−310/11 viewed
+33−1
223223
224224 When a newer version comes out for a dependency (or group) that already
225225 has an open pull request, g1t opens a new pull request and closes the
226−older one with the comment "Superseded by #N.".
226+older one with the comment "Closed: superseded by #N.". It deletes the
227+older pull request's branch.
228+
229+### Updates you make yourself
230+
231+You do not have to merge g1t's pull request to update a dependency. On
232+every push to the default branch, g1t reads the lockfiles again. When
233+every dependency an open version update raises is already at its new
234+version or later, or is no longer a dependency, g1t closes the pull
235+request with a comment that says so, for example:
236+
237+> Closed: `lodash` is already at 4.17.21 on `main`, so this update to
238+> 4.17.21 is no longer needed.
239+
240+g1t then deletes the pull request's branch. While one dependency in a
241+grouped pull request still needs it, the pull request stays open. This
242+applies to updates into the default branch; one with a `target-branch`
243+is left for you to close.
244+
245+### Branches
246+
247+Each update pull request is made on a branch g1t creates (see
248+[branch names](#pull-request-branch-name)). When the pull request merges
249+or closes, whether g1t or a person closes it, g1t deletes that branch. If
250+someone pushed to the branch after its last commit in the pull request,
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.
255+
256+Deleting the branch means a closed update pull request cannot be reopened
257+from the page. To make a version update again, comment `@g1t reopen` on
258+it: g1t makes it again as a new pull request.
227259
228260 ### Landing them
229261
+39−2
299299 | Open | The pull request is open. |
300300 | Merged | The pull request merged. |
301301 | Closed | The pull request was closed without merging. |
302−| Superseded | A newer security update for the same package replaced it, or the package is no longer vulnerable. |
302+| Superseded | A newer security update for the same package replaced it, or the alerts it fixes were fixed another way or dismissed. |
303303 | Needs code changes | Raising the version was not enough; an agent is working on it. |
304304 | Failed | The update could not be made. |
305305
306+### When an update is no longer needed
307+
306308 A newer security update for the same package closes the older pull request
307−as superseded. So does the package no longer being vulnerable.
309+with the comment "Closed: superseded by #N, which upgrades `<package>` to
310+`<version>`.".
311+
312+g1t also closes a security update once none of the alerts it fixes is
313+open. An alert counts as one the update fixes when it is on the update's
314+package, at a version below the one the update raises it to. Each of those
315+alerts must be:
316+
317+- **Fixed on the default branch.** You raised the version another way, for
318+ example with an `overrides` entry in `package.json` or by updating the
319+ lockfile yourself, and pushed it. The next scan no longer finds the
320+ vulnerable version in the lockfiles.
321+- **Dismissed.** g1t checks when you dismiss an alert, without waiting for a
322+ push.
323+
324+g1t comments why, closes the pull request and deletes its branch:
325+
326+> Closed: the alert this fixed is resolved on `main` (`sharp` 0.34.1 is no
327+> longer in `package-lock.json`).
328+
329+> Closed: the alert this fixed was dismissed, so this update is no longer
330+> needed.
331+
332+A grouped security update closes only when the alerts of every package in
333+it are fixed or dismissed. If a package is still vulnerable at a higher
334+version than the pull request reached, g1t closes the pull request and
335+asks for a new one for the higher version in the same scan. A later scan never
336+opens the closed pull request again. g1t opens a new one only if the
337+package becomes vulnerable again, at a version that update would fix.
338+
339+When a security update pull request merges or closes, by g1t or by a
340+person, g1t deletes its `g1t/security/…` branch, unless someone pushed to
341+it after its last commit in the pull request. A branch left from an
342+update pull request that was already closed is removed on a later push to
343+the default branch. See
344+[branches](/guides/dependency-updates/#branches).
308345
309346 How each lockfile is changed:
310347
+8−2
795795
796796 /// `delete_branch`: removes a branch g1t made for itself once it is done
797797 /// with it, never one of people's: the name must start with
798−/// [`G1T_BRANCH_PREFIX`]. For services, which have no viewer. Returns
799−/// `Outcome<bool>`: whether there was such a branch.
798+/// [`G1T_BRANCH_PREFIX`], or `head` must name the commit it points to (a
799+/// dependency update's branch, which g1t pushed and whose pull request it
800+/// closed). Never the default branch. For services, which have no viewer.
801+/// Returns `Outcome<bool>`: whether there was such a branch.
800802 #[derive(Debug, Serialize, Deserialize)]
801803 #[serde(rename_all = "camelCase")]
802804 pub struct DeleteBranchArgs {
803805 pub repo_id: String,
804806 pub branch: String,
807+ /// The commit the branch must still point to. A branch that moved
808+ /// since (someone pushed to it) is left alone.
809+ #[serde(default, skip_serializing_if = "Option::is_none")]
810+ pub head: Option<String>,
805811 }
806812
807813 /// `commit_file`: writes one file on a new branch made from the default
+29−1
12121212 }
12131213
12141214 async fn delete_branch(&self, a: DeleteBranchArgs) -> Result<Outcome<bool>> {
1215− if !a.branch.starts_with(G1T_BRANCH_PREFIX) {
1215+ if !deletable_branch(&a.branch, a.head.as_deref()) {
12161216 return Ok(Outcome::fail(
12171217 FailureCode::Forbidden,
12181218 "Only branches g1t made for itself can be deleted this way.",
12211221 let Some(repo) = self.registry.by_id(&a.repo_id).await? else {
12221222 return Ok(not_found());
12231223 };
1224+ if a.branch == repo.default_branch {
1225+ return Ok(Outcome::fail(FailureCode::Forbidden, "The default branch is never deleted."));
1226+ }
12241227 let repo = match self.unpaused(repo).await? {
12251228 Ok(repo) => repo,
12261229 Err((code, message)) => return Ok(Outcome::fail(code, message)),
12361239 else {
12371240 return Ok(Outcome::Ok(false));
12381241 };
1242+ // Moved since the caller looked: someone else's commits are on it.
1243+ if a.head.as_deref().is_some_and(|head| head != old) {
1244+ return Ok(Outcome::fail(FailureCode::Conflict, format!("{} moved, so it was left alone.", a.branch)));
1245+ }
12391246 let access = git.access(Scope::Write).await?;
12401247 let deleted = land::delete_ref(&access, &a.branch, &old).await?;
12411248 self.refs_moved(&repo.id).await;
27842791 }
27852792 }
27862793
2794+/// Whether `delete_branch` may remove `branch`: one of g1t's own
2795+/// (`g1t-…`), or one whose tip the caller names, such as a dependency
2796+/// update's branch after its pull request closed.
2797+fn deletable_branch(branch: &str, head: Option<&str>) -> bool {
2798+ !branch.is_empty() && (branch.starts_with(G1T_BRANCH_PREFIX) || head.is_some_and(|head| !head.is_empty()))
2799+}
2800+
2801+#[cfg(test)]
2802+mod delete_branch_tests {
2803+ use super::deletable_branch;
2804+
2805+ #[test]
2806+ fn only_g1t_branches_or_a_named_tip_are_deleted() {
2807+ assert!(deletable_branch("g1t-queue-12", None));
2808+ assert!(!deletable_branch("g1t/security/sharp-0.35.5", None));
2809+ assert!(deletable_branch("g1t/security/sharp-0.35.5", Some("abc123")));
2810+ assert!(!deletable_branch("feature", Some("")));
2811+ assert!(!deletable_branch("", Some("abc123")));
2812+ }
2813+}
2814+
27872815 #[cfg(test)]
27882816 mod push_to_create_tests {
27892817 use super::*;
+6−0
215215 let before: BTreeSet<String> = self.store.open_vulnerabilities(&repo.repo_id).await?.into_iter().map(|row| row.id).collect();
216216 self.store.replace_vulnerabilities(&repo.repo_id, &found).await?;
217217 self.store.set_dependencies_scanned(&repo.repo_id, files.commit.as_deref(), &paths, None).await?;
218+ // g1t's update pull requests whose alerts are now fixed or dismissed,
219+ // or whose versions the lockfiles already have, close first, so that
220+ // nothing later in the scan can keep them open.
221+ if let Err(error) = self.resolve_updates(repo, Some((&located, &paths))).await {
222+ worker::console_error!("security: updates of {} not resolved: {error}", repo.repo_id);
223+ }
218224 if let Err(error) = self.record_graph(repo, &files).await {
219225 worker::console_error!("security: dependency graph of {} not kept: {error}", repo.repo_id);
220226 }
+13−1
3939 mod patterns;
4040 mod planning;
4141 mod pull_text;
42+mod resolved;
4243 mod ranges;
4344 mod registries;
4445 mod schedule;
323324 let event = g1t_contracts::security_suite::SecurityEvent { reason: Some(a.reason.as_str().to_owned()), ..deps::vulnerability_event(&repo, vuln) };
324325 self.alert_event(g1t_contracts::security_suite::AlertType::Vulnerability, "dismissed", &repo, event, Some(a.actor.id.clone())).await;
325326 }
327+ // A security update whose alerts are now all fixed or dismissed closes.
328+ if let Err(error) = self.resolve_updates(&repo, None).await {
329+ worker::console_error!("security: updates of {} not resolved after a dismissal: {error}", repo.repo_id);
330+ }
326331 Ok(Outcome::Ok(AlertChange { secret: None, vulnerability }))
327332 }
328333
515520 if let Some(branch) = pushed.git_ref.strip_prefix("refs/heads/")
516521 && branch.starts_with(UPDATE_BRANCH_PREFIX)
517522 {
518− self.update_pushed(&pushed.repo_id, branch).await?;
523+ self.update_pushed(&pushed.repo_id, branch, &pushed.after).await?;
519524 return Ok(());
520525 }
521526 if !pushed.default_branch {
541546 )
542547 .await?;
543548 }
549+ // One of g1t's update pull requests closed or merged, by g1t
550+ // or by a person: its branch goes.
551+ if event.kind != "checks.completed"
552+ && let Ok(happened) = serde_json::from_value::<PullHappened>(event.data.clone())
553+ {
554+ self.pull_finished(&happened.repo_id, happened.number).await?;
555+ }
544556 }
545557 "comment.created" => self.update_comment(event).await?,
546558 // A pull request opened or its head moved: a dependency update
+515−0
1+//! Update pull requests that are no longer needed, and the branches g1t
2+//! leaves behind.
3+//!
4+//! After every dependency scan of the default branch, and when someone
5+//! dismisses an alert, g1t looks at its own open update pull requests:
6+//!
7+//! - A security update (one package, or a `security-updates` group) is
8+//! closed once none of the alerts it fixes is open: each was fixed on the
9+//! default branch (its vulnerable version left the lockfiles) or
10+//! dismissed. An alert counts as one the update fixes when it is on the
11+//! update's package at a version below the update's target.
12+//! - A version update into the default branch is closed once every
13+//! dependency it raises is already at its new version or later in the
14+//! lockfiles, or is no longer a dependency.
15+//!
16+//! g1t comments why, closes the pull request and deletes its branch. The
17+//! update is recorded as superseded, so a later scan asks for a new one
18+//! only when one of its packages is vulnerable again.
19+//!
20+//! Branches: when one of these pull requests closes or merges, by g1t or
21+//! by a person, its branch is deleted, and each scan removes those still
22+//! left from update pull requests already closed. Only a branch g1t made
23+//! for an update, with no update in progress on it, still at the commit its
24+//! pull request last had, is deleted.
25+
26+use std::collections::{BTreeMap, BTreeSet};
27+
28+use g1t_contracts::repos::{Branch, BranchesArgs, DeleteBranchArgs};
29+use g1t_contracts::security::UpdateState;
30+use g1t_contracts::updates::UpdatedDependency;
31+use g1t_contracts::work::{Pull, PullStatus};
32+use g1t_contracts::{Outcome, User};
33+use g1t_scan::version;
34+use worker::Result;
35+
36+use crate::Security;
37+use crate::deps::Located;
38+use crate::store::{Activity, RepoRow, VulnRow};
39+use crate::version_updates::{lockfiles_for, normalize, osv_ecosystem};
40+
41+/// Branches of closed update pull requests removed per scan, at most.
42+const BRANCHES_PER_SCAN: usize = 10;
43+
44+/// The alerts an update to `target` fixes: those on `package` at a lower
45+/// version, whatever their state now.
46+pub fn targeted<'a>(alerts: &'a [VulnRow], ecosystem: &str, package: &str, target: &str) -> Vec<&'a VulnRow> {
47+ alerts
48+ .iter()
49+ .filter(|alert| alert.ecosystem == ecosystem && alert.package == package && version::compare(&alert.version, target).is_lt())
50+ .collect()
51+}
52+
53+/// Whether one of the alerts an update fixes is still open.
54+pub fn still_needed(targeted: &[&VulnRow]) -> bool {
55+ targeted.iter().any(|alert| alert.status == "open")
56+}
57+
58+fn join(items: &[String], last: &str) -> String {
59+ match items {
60+ [] => String::new(),
61+ [one] => one.clone(),
62+ [rest @ .., end] => format!("{} {last} {end}", rest.join(", ")),
63+ }
64+}
65+
66+/// The comment g1t closes a security update with once none of the alerts
67+/// it fixes is open. `packages` names the update's packages, for when no
68+/// alert is left to name.
69+pub fn resolved_text(base: &str, packages: &[String], targeted: &[&VulnRow]) -> String {
70+ let fixed: Vec<&&VulnRow> = targeted.iter().filter(|alert| alert.status == "fixed").collect();
71+ let dismissed = targeted.iter().any(|alert| alert.status == "dismissed");
72+ let advisories: BTreeSet<&str> = targeted.iter().map(|alert| alert.osv_id.as_str()).collect();
73+ let (one, plural) = (advisories.len() <= 1, advisories.len() > 1);
74+ let subject = if one { "the alert this fixed" } else { "the alerts this fixed" };
75+ if fixed.is_empty() && !dismissed {
76+ let names: Vec<String> = packages.iter().map(|name| format!("`{name}`")).collect();
77+ return format!(
78+ "Closed: {} no longer vulnerable on `{base}`, so this update is no longer needed.",
79+ if names.len() == 1 { format!("{} is", names[0]) } else { format!("{} are", join(&names, "and")) }
80+ );
81+ }
82+ if fixed.is_empty() {
83+ return format!("Closed: {subject} {} dismissed, so this update is no longer needed.", if plural { "were" } else { "was" });
84+ }
85+ // Each package with the vulnerable versions that left, and where from.
86+ let mut versions: BTreeMap<&str, BTreeSet<&str>> = BTreeMap::new();
87+ let mut lockfiles: BTreeSet<&str> = BTreeSet::new();
88+ for alert in &fixed {
89+ versions.entry(alert.package.as_str()).or_default().insert(alert.version.as_str());
90+ lockfiles.insert(alert.manifest.as_str());
91+ }
92+ let count: usize = versions.values().map(BTreeSet::len).sum();
93+ let named: Vec<String> = versions
94+ .iter()
95+ .map(|(package, found)| {
96+ let found: Vec<String> = found.iter().map(|v| (*v).to_owned()).collect();
97+ format!("`{package}` {}", join(&found, "and"))
98+ })
99+ .collect();
100+ let files: Vec<String> = lockfiles.iter().map(|path| format!("`{path}`")).collect();
101+ format!(
102+ "Closed: {subject} {} resolved on `{base}`{} ({} {} no longer in {}).",
103+ if one { "is" } else { "are" },
104+ if dismissed { " or dismissed" } else { "" },
105+ join(&named, "and"),
106+ if count == 1 { "is" } else { "are" },
107+ join(&files, "or"),
108+ )
109+}
110+
111+/// Where a dependency a version update raises stands on the default branch.
112+#[derive(Debug, Clone, PartialEq, Eq)]
113+pub enum Now {
114+ /// The highest version its lockfiles resolve.
115+ At(String),
116+ /// Its lockfiles no longer have it.
117+ Gone,
118+ /// No lockfile for its directory could be read: nothing is decided.
119+ Unknown,
120+}
121+
122+/// Where `dependency` stands in the lockfiles a scan read.
123+pub fn now_of(ecosystem: &str, dependency: &UpdatedDependency, located: &[Located], paths: &[String]) -> Now {
124+ let lockfiles = lockfiles_for(ecosystem, &dependency.directory, paths);
125+ let read: Vec<&Located> = located.iter().filter(|item| lockfiles.contains(&item.path)).collect();
126+ if read.is_empty() {
127+ return Now::Unknown;
128+ }
129+ let name = normalize(ecosystem, &dependency.name);
130+ read.iter()
131+ .filter(|item| normalize(ecosystem, &item.package.name) == name)
132+ .map(|item| item.package.version.clone())
133+ .max_by(|a, b| version::compare(a, b))
134+ .map_or(Now::Gone, Now::At)
135+}
136+
137+/// The comment g1t closes a version update with when every dependency it
138+/// raises is already there, or `None` while one still needs it.
139+pub fn version_resolved_text(base: &str, dependencies: &[(UpdatedDependency, Now)]) -> Option<String> {
140+ if dependencies.is_empty() {
141+ return None;
142+ }
143+ for (dependency, now) in dependencies {
144+ match now {
145+ Now::Unknown => return None,
146+ Now::At(current) if version::compare(current, &dependency.to).is_lt() => return None,
147+ _ => {}
148+ }
149+ }
150+ if let [(dependency, now)] = dependencies {
151+ return Some(match now {
152+ Now::At(current) => format!(
153+ "Closed: `{}` is already at {current} on `{base}`, so this update to {} is no longer needed.",
154+ dependency.name, dependency.to
155+ ),
156+ _ => format!("Closed: `{}` is no longer a dependency on `{base}`, so this update is no longer needed.", dependency.name),
157+ });
158+ }
159+ let each: Vec<String> = dependencies
160+ .iter()
161+ .map(|(dependency, now)| match now {
162+ Now::At(current) => format!("`{}` is at {current}", dependency.name),
163+ _ => format!("`{}` is no longer a dependency", dependency.name),
164+ })
165+ .collect();
166+ Some(format!(
167+ "Closed: every dependency this updates is already at its new version or later on `{base}` ({}), so this update is no longer needed.",
168+ join(&each, "and")
169+ ))
170+}
171+
172+/// The closing comment, given the pull request's base branch.
173+type Why<'a> = Box<dyn FnOnce(&str) -> String + 'a>;
174+
175+/// What closing a no longer needed update pull request found.
176+#[derive(Debug, PartialEq, Eq)]
177+pub enum Retired {
178+ /// It was open: g1t commented, closed it and deleted its branch.
179+ Closed,
180+ /// It merged meanwhile.
181+ Merged,
182+ /// Already closed, or not found.
183+ Left,
184+}
185+
186+impl Security {
187+ /// Closes one of g1t's update pull requests that is no longer needed,
188+ /// with the comment `why` gives for its base branch, and deletes its
189+ /// branch.
190+ pub(crate) async fn retire(&self, repo: &RepoRow, number: u32, why: impl FnOnce(&str) -> String) -> Result<Retired> {
191+ let Some(pull) = self.get_pull(repo, number).await? else { return Ok(Retired::Left) };
192+ match pull.status {
193+ PullStatus::Merged => Ok(Retired::Merged),
194+ PullStatus::Closed => Ok(Retired::Left),
195+ PullStatus::Open | PullStatus::Draft => {
196+ let base = pull.base.clone().unwrap_or_else(|| "the default branch".to_owned());
197+ if self.close_with(repo, number, why(&base)).await? {
198+ let closed = Pull { status: PullStatus::Closed, ..pull };
199+ self.delete_pull_branch(repo, &closed).await;
200+ }
201+ Ok(Retired::Closed)
202+ }
203+ }
204+ }
205+
206+ /// Deletes a closed or merged update pull request's branch, if it is
207+ /// still at the pull request's last commit. Failures are logged: a
208+ /// branch left behind is removed by a later scan.
209+ pub(crate) async fn delete_pull_branch(&self, repo: &RepoRow, pull: &Pull) {
210+ if matches!(pull.status, PullStatus::Open | PullStatus::Draft) || pull.fork.is_some() || pull.fork_repo_id.is_some() {
211+ return;
212+ }
213+ let (Some(branch), Some(head)) = (pull.branch.clone(), pull.head_commit.clone()) else { return };
214+ if pull.base.as_deref() == Some(branch.as_str()) {
215+ return;
216+ }
217+ let deleted: Result<Outcome<bool>> =
218+ g1t_kit::call(&self.repos, "delete_branch", &DeleteBranchArgs { repo_id: repo.repo_id.clone(), branch: branch.clone(), head: Some(head) }).await;
219+ match deleted {
220+ Ok(Outcome::Ok(_)) => {}
221+ Ok(Outcome::Fail(refused)) => worker::console_log!("security: branch {branch} of {} kept: {}", repo.repo_id, refused.message),
222+ Err(error) => worker::console_error!("security: branch {branch} of {} not deleted: {error}", repo.repo_id),
223+ }
224+ }
225+
226+ /// An update branch pushed by its sandbox after the update stopped
227+ /// being needed: deleted, while still at what the sandbox pushed.
228+ pub(crate) async fn drop_pushed(&self, repo_id: &str, branch: &str, after: &str) -> Result<()> {
229+ if after.is_empty() || after.bytes().all(|byte| byte == b'0') {
230+ return Ok(());
231+ }
232+ let deleted: Outcome<bool> = g1t_kit::call(
233+ &self.repos,
234+ "delete_branch",
235+ &DeleteBranchArgs { repo_id: repo_id.to_owned(), branch: branch.to_owned(), head: Some(after.to_owned()) },
236+ )
237+ .await?;
238+ if let Outcome::Fail(refused) = deleted {
239+ worker::console_log!("security: branch {branch} of {repo_id} kept: {}", refused.message);
240+ }
241+ Ok(())
242+ }
243+
244+ /// A pull request closed or merged: if it is one of g1t's update pull
245+ /// requests, its branch goes.
246+ pub(crate) async fn pull_finished(&self, repo_id: &str, number: u32) -> Result<()> {
247+ let ours = self.store.update_by_pull(repo_id, number).await?.is_some() || self.store.update_pull_by_number(repo_id, number).await?.is_some();
248+ if !ours {
249+ return Ok(());
250+ }
251+ let Some(repo) = self.store.repo(repo_id).await? else { return Ok(()) };
252+ if let Some(pull) = self.get_pull(&repo, number).await?
253+ && !self.branch_in_use(&repo.repo_id, pull.branch.as_deref().unwrap_or_default()).await?
254+ {
255+ self.delete_pull_branch(&repo, &pull).await;
256+ }
257+ Ok(())
258+ }
259+
260+ /// Whether an update still in progress is made on `branch`.
261+ async fn branch_in_use(&self, repo_id: &str, branch: &str) -> Result<bool> {
262+ let single = self.store.updates(repo_id).await?.into_iter().any(|row| row.state().in_progress() && row.branch.as_deref() == Some(branch));
263+ let pulls = self.store.update_pulls(repo_id).await?.into_iter().any(|row| row.state().in_progress() && row.branch == branch);
264+ Ok(single || pulls)
265+ }
266+
267+ /// Closes g1t's update pull requests that are no longer needed (see the
268+ /// module), then removes branches left by closed ones. `lockfiles` is
269+ /// what a scan of the default branch read, when one did: without it,
270+ /// version updates are left as they are.
271+ pub(crate) async fn resolve_updates(&self, repo: &RepoRow, lockfiles: Option<(&[Located], &[String])>) -> Result<()> {
272+ let alerts = self.store.all_vulnerabilities(&repo.repo_id).await?;
273+ let pulls = self.store.update_pulls(&repo.repo_id).await?;
274+ let grouped: BTreeSet<u32> = pulls.iter().filter(|row| row.kind == "security").filter_map(|row| row.pull()).collect();
275+ let system = g1t_contracts::system::USERNAME;
276+ // One package's security update.
277+ for row in self.store.updates(&repo.repo_id).await? {
278+ if !row.state().in_progress() || row.pull().is_some_and(|number| grouped.contains(&number)) {
279+ continue;
280+ }
281+ let fixes = targeted(&alerts, &row.ecosystem, &row.package, &row.target);
282+ if still_needed(&fixes) {
283+ continue;
284+ }
285+ self.store.set_update(&row, UpdateState::Superseded, row.pull(), row.issue(), None).await?;
286+ let Some(number) = row.pull() else { continue };
287+ let packages = [row.package.clone()];
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?,
290+ Retired::Closed => {
291+ let activity: Vec<Activity> =
292+ fixes.iter().map(|alert| Activity { alert_id: &alert.id, action: "update_superseded", actor: Some(system), reason: None, comment: None, number: Some(number) }).collect();
293+ self.store.record(&repo.repo_id, &activity).await?;
294+ }
295+ Retired::Left => {}
296+ }
297+ }
298+ // Grouped security updates, and version updates.
299+ for row in &pulls {
300+ if !matches!(row.state(), UpdateState::Open | UpdateState::Requested) {
301+ continue;
302+ }
303+ let dependencies = row.dependencies();
304+ let why: Option<Why<'_>> = if row.kind == "security" {
305+ let Some(osv) = osv_ecosystem(&row.ecosystem) else { continue };
306+ let fixes: Vec<&VulnRow> = dependencies.iter().flat_map(|dependency| targeted(&alerts, osv, &dependency.name, &dependency.to)).collect();
307+ if dependencies.is_empty() || still_needed(&fixes) {
308+ None
309+ } else {
310+ let packages: Vec<String> = dependencies.iter().map(|dependency| dependency.name.clone()).collect();
311+ Some(Box::new(move |base: &str| resolved_text(base, &packages, &fixes)))
312+ }
313+ } else {
314+ // Only into the default branch, which the scan read, and
315+ // only once its pull request is open.
316+ let into_default = row.bump().is_none_or(|bump| bump.base.is_none());
317+ match lockfiles {
318+ Some((located, paths)) if into_default && row.state() == UpdateState::Open => {
319+ let now: Vec<(UpdatedDependency, Now)> =
320+ dependencies.iter().map(|dependency| (dependency.clone(), now_of(&row.ecosystem, dependency, located, paths))).collect();
321+ version_resolved_text("", &now).is_some().then(|| {
322+ Box::new(move |base: &str| version_resolved_text(base, &now).unwrap_or_default()) as Why<'_>
323+ })
324+ }
325+ _ => None,
326+ }
327+ };
328+ let Some(why) = why else { continue };
329+ self.store.set_update_pull(&row.id, UpdateState::Superseded, row.pull(), None, None).await?;
330+ let retired = match row.pull() {
331+ Some(number) => self.retire(repo, number, why).await?,
332+ None => Retired::Left,
333+ };
334+ if retired == Retired::Merged {
335+ self.store.set_update_pull(&row.id, UpdateState::Merged, row.pull(), None, None).await?;
336+ }
337+ // A grouped security update stands for each package's own.
338+ if row.kind == "security"
339+ && let Some(osv) = osv_ecosystem(&row.ecosystem)
340+ {
341+ let state = if retired == Retired::Merged { UpdateState::Merged } else { UpdateState::Superseded };
342+ for dependency in &dependencies {
343+ if let Some(update) = self.store.update(&repo.repo_id, osv, &dependency.name).await?
344+ && (update.state().in_progress() && (update.pull() == row.pull() || update.branch.as_deref() == Some(row.branch.as_str())))
345+ {
346+ self.store.set_update(&update, state, update.pull(), update.issue(), None).await?;
347+ }
348+ }
349+ }
350+ }
351+ if let Err(error) = self.clean_update_branches(repo).await {
352+ worker::console_error!("security: update branches of {} not cleaned: {error}", repo.repo_id);
353+ }
354+ Ok(())
355+ }
356+
357+ /// Removes branches left by g1t's update pull requests that are already
358+ /// closed or merged: a few per scan, only those no update in progress
359+ /// uses, and only while they are at the pull request's last commit.
360+ async fn clean_update_branches(&self, repo: &RepoRow) -> Result<()> {
361+ let updates = self.store.updates(&repo.repo_id).await?;
362+ let pulls = self.store.update_pulls(&repo.repo_id).await?;
363+ let finished = |state: UpdateState| matches!(state, UpdateState::Closed | UpdateState::Merged | UpdateState::Superseded);
364+ let in_use: BTreeSet<&str> = updates
365+ .iter()
366+ .filter(|row| row.state().in_progress())
367+ .filter_map(|row| row.branch.as_deref())
368+ .chain(pulls.iter().filter(|row| row.state().in_progress()).map(|row| row.branch.as_str()))
369+ .collect();
370+ let mut candidates: BTreeMap<&str, u32> = BTreeMap::new();
371+ for (branch, number) in updates
372+ .iter()
373+ .filter(|row| finished(row.state()))
374+ .filter_map(|row| Some((row.branch.as_deref()?, row.pull()?)))
375+ .chain(pulls.iter().filter(|row| finished(row.state())).filter_map(|row| Some((row.branch.as_str(), row.pull()?))))
376+ {
377+ if !branch.is_empty() && !in_use.contains(branch) {
378+ candidates.entry(branch).or_insert(number);
379+ }
380+ }
381+ if candidates.is_empty() {
382+ return Ok(());
383+ }
384+ let listed: Outcome<Vec<Branch>> = g1t_kit::call(
385+ &self.repos,
386+ "branches",
387+ &BranchesArgs { path: Self::path_of(repo), viewer: Some(User::system(&repo.namespace)) },
388+ )
389+ .await?;
390+ let Outcome::Ok(branches) = listed else { return Ok(()) };
391+ let tips: BTreeMap<&str, &str> = branches.iter().map(|branch| (branch.name.as_str(), branch.hash.as_str())).collect();
392+ let mut looked = 0;
393+ for (branch, number) in candidates {
394+ if looked >= BRANCHES_PER_SCAN {
395+ break;
396+ }
397+ let Some(tip) = tips.get(branch) else { continue };
398+ looked += 1;
399+ let Some(pull) = self.get_pull(repo, number).await? else { continue };
400+ if pull.branch.as_deref() == Some(branch) && pull.head_commit.as_deref() == Some(*tip) {
401+ self.delete_pull_branch(repo, &pull).await;
402+ }
403+ }
404+ Ok(())
405+ }
406+}
407+
408+#[cfg(test)]
409+mod tests {
410+ use super::*;
411+ use g1t_scan::lockfiles::{Lockfile, Package};
412+
413+ fn alert(package: &str, version: &str, manifest: &str, osv: &str, status: &str) -> VulnRow {
414+ VulnRow {
415+ id: format!("vul_{package}_{version}_{osv}"),
416+ repo_id: "rep_1".into(),
417+ ecosystem: "npm".into(),
418+ package: package.into(),
419+ version: version.into(),
420+ manifest: manifest.into(),
421+ osv_id: osv.into(),
422+ advisory: osv.into(),
423+ summary: String::new(),
424+ severity: "high".into(),
425+ fixed_version: Some("0.35.5".into()),
426+ status: status.into(),
427+ found_at: "2026-10-04T00:00:00Z".into(),
428+ fixed_at: None,
429+ number: None,
430+ dismiss_reason: None,
431+ dismiss_comment: None,
432+ dismissed_by: None,
433+ dismissed_at: None,
434+ }
435+ }
436+
437+ #[test]
438+ fn an_update_is_needed_while_an_alert_below_its_target_is_open() {
439+ let alerts = vec![
440+ alert("sharp", "0.34.1", "package-lock.json", "GHSA-1", "open"),
441+ // At or above the target: another update's business.
442+ alert("sharp", "0.35.5", "package-lock.json", "GHSA-2", "open"),
443+ alert("lodash", "4.17.20", "package-lock.json", "GHSA-3", "open"),
444+ ];
445+ let fixes = targeted(&alerts, "npm", "sharp", "0.35.5");
446+ assert_eq!(fixes.len(), 1);
447+ assert!(still_needed(&fixes));
448+ let alerts = vec![alert("sharp", "0.34.1", "package-lock.json", "GHSA-1", "fixed"), alert("sharp", "0.35.5", "package-lock.json", "GHSA-2", "open")];
449+ let fixes = targeted(&alerts, "npm", "sharp", "0.35.5");
450+ assert!(!still_needed(&fixes), "an alert the update would not fix keeps nothing open");
451+ assert!(targeted(&alerts, "crates.io", "sharp", "0.35.5").is_empty());
452+ }
453+
454+ #[test]
455+ fn the_closing_comment_says_why() {
456+ let packages = ["sharp".to_owned()];
457+ let fixed = alert("sharp", "0.34.1", "package-lock.json", "GHSA-1", "fixed");
458+ assert_eq!(
459+ resolved_text("main", &packages, &[&fixed]),
460+ "Closed: the alert this fixed is resolved on `main` (`sharp` 0.34.1 is no longer in `package-lock.json`)."
461+ );
462+ let other = alert("sharp", "0.33.0", "web/package-lock.json", "GHSA-9", "fixed");
463+ assert_eq!(
464+ resolved_text("main", &packages, &[&fixed, &other]),
465+ "Closed: the alerts this fixed are resolved on `main` (`sharp` 0.33.0 and 0.34.1 are no longer in `package-lock.json` or `web/package-lock.json`)."
466+ );
467+ let dismissed = alert("sharp", "0.34.1", "package-lock.json", "GHSA-9", "dismissed");
468+ assert_eq!(resolved_text("main", &packages, &[&dismissed]), "Closed: the alert this fixed was dismissed, so this update is no longer needed.");
469+ assert!(resolved_text("main", &packages, &[&fixed, &dismissed]).starts_with("Closed: the alerts this fixed are resolved on `main` or dismissed ("));
470+ assert_eq!(resolved_text("trunk", &packages, &[]), "Closed: `sharp` is no longer vulnerable on `trunk`, so this update is no longer needed.");
471+ }
472+
473+ fn dependency(name: &str, to: &str, directory: &str) -> UpdatedDependency {
474+ UpdatedDependency {
475+ name: name.into(),
476+ from: "1.0.0".into(),
477+ to: to.into(),
478+ directory: directory.into(),
479+ dependency_type: "direct:production".into(),
480+ update_type: "version-update:semver-minor".into(),
481+ }
482+ }
483+
484+ fn located(name: &str, version: &str, path: &str) -> Located {
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() }
487+ }
488+
489+ #[test]
490+ fn a_version_update_is_done_once_the_lockfile_has_its_version() {
491+ let paths = vec!["package-lock.json".to_owned(), "apps/web/package.json".to_owned()];
492+ let read = vec![located("lodash", "4.17.21", "package-lock.json"), located("react", "18.2.0", "package-lock.json")];
493+ let lodash = dependency("lodash", "4.17.21", "/");
494+ let react = dependency("react", "18.3.1", "/apps/web");
495+ let gone = dependency("left-pad", "1.3.0", "/");
496+ // A workspace directory is resolved by the lockfile above it.
497+ assert_eq!(now_of("npm", &react, &read, &paths), Now::At("18.2.0".into()));
498+ assert_eq!(now_of("npm", &gone, &read, &paths), Now::Gone);
499+ assert_eq!(now_of("cargo", &dependency("serde", "1.0.200", "/"), &read, &paths), Now::Unknown);
500+ let at = now_of("npm", &lodash, &read, &paths);
501+ assert_eq!(
502+ version_resolved_text("main", &[(lodash.clone(), at.clone())]).as_deref(),
503+ Some("Closed: `lodash` is already at 4.17.21 on `main`, so this update to 4.17.21 is no longer needed.")
504+ );
505+ assert_eq!(
506+ version_resolved_text("main", &[(gone.clone(), Now::Gone)]).as_deref(),
507+ Some("Closed: `left-pad` is no longer a dependency on `main`, so this update is no longer needed.")
508+ );
509+ // One dependency still behind keeps the whole pull request.
510+ assert_eq!(version_resolved_text("main", &[(lodash.clone(), at.clone()), (react.clone(), Now::At("18.2.0".into()))]), None);
511+ assert_eq!(version_resolved_text("main", &[(lodash.clone(), Now::Unknown)]), None);
512+ let both = version_resolved_text("main", &[(lodash, at), (gone, Now::Gone)]).unwrap();
513+ assert!(both.contains("`lodash` is at 4.17.21 and `left-pad` is no longer a dependency"), "{both}");
514+ }
515+}
+29−21
1111 //! never pushed, the pull request is closed and an issue is opened for
1212 //! g1t to work on, started by g1t. That is the only time an agent is
1313 //! used.
14−//! 4. A package no longer vulnerable closes its update as superseded.
14+//! 4. Once none of the alerts it fixes is open (fixed on the default
15+//! branch, or dismissed), its pull request is closed with a comment
16+//! saying why and its branch deleted (`resolved`).
1517 //!
1618 //! Each step is written to the alerts' activity log.
1719
209211 }
210212
211213 impl Security {
212− fn path_of(repo: &RepoRow) -> RepoPath {
214+ pub(crate) fn path_of(repo: &RepoRow) -> RepoPath {
213215 RepoPath { namespace: repo.namespace.clone(), name: repo.name.clone() }
214216 }
215217
216− async fn get_pull(&self, repo: &RepoRow, number: u32) -> Result<Option<Pull>> {
218+ pub(crate) async fn get_pull(&self, repo: &RepoRow, number: u32) -> Result<Option<Pull>> {
217219 let found: Outcome<PullDetail> = g1t_kit::call(
218220 &self.work,
219221 "get_pull",
223225 Ok(found.into_result().ok().map(|detail| detail.pull))
224226 }
225227
226− /// Closes one of g1t's pull requests, saying why first.
227− pub(crate) async fn close_with(&self, repo: &RepoRow, number: u32, why: String) -> Result<()> {
228+ /// Closes one of g1t's pull requests, saying why first. Returns
229+ /// whether it closed.
230+ pub(crate) async fn close_with(&self, repo: &RepoRow, number: u32, why: String) -> Result<bool> {
228231 let system = User::system(&repo.namespace);
229232 self.comment(&system, &Self::path_of(repo), number, why).await?;
230− let _: Outcome<Value> = g1t_kit::call(
233+ let closed: Outcome<Value> = g1t_kit::call(
231234 &self.work,
232235 "close_pull",
233236 &PullActionArgs {
241244 },
242245 )
243246 .await?;
244− Ok(())
247+ if let Outcome::Fail(refused) = &closed {
248+ worker::console_error!("security: #{number} of {} not closed: {}", repo.repo_id, refused.message);
249+ }
250+ Ok(matches!(closed, Outcome::Ok(_)))
245251 }
246252
247253 /// Records `action` on every open alert of an update's package.
255261 }
256262
257263 /// After a dependency scan: asks for an update for each vulnerable
258− /// package with a fix (when `enabled`), and supersedes those whose
259− /// package is no longer vulnerable.
264+ /// package with a fix (when `enabled`). Those no longer needed were
265+ /// closed already (`resolved`).
260266 ///
261267 /// The dependency update file, when there is one, applies as it does to
262268 /// version updates: `ignore` and `allow` by name, people's `@g1t ignore`
268274 let mut by_package: BTreeMap<(String, String), Vec<&VulnRow>> = BTreeMap::new();
269275 for vuln in &open {
270276 by_package.entry((vuln.ecosystem.clone(), vuln.package.clone())).or_default().push(vuln);
271− }
272− // In flight for a package that is no longer vulnerable: no longer needed.
273− for row in self.store.updates(&repo.repo_id).await? {
274− if !row.state().in_progress() || by_package.contains_key(&(row.ecosystem.clone(), row.package.clone())) {
275− continue;
276− }
277− self.supersede(repo, &row, format!("`{}` is no longer vulnerable here, so this is no longer needed.", row.package))
278− .await?;
279277 }
280278 if !enabled {
281279 return Ok(());
477475
478476 /// A security update's branch was pushed: its pull request opens, and
479477 /// an older one for the same package is closed as superseded.
480− pub async fn update_pushed(&self, repo_id: &str, branch: &str) -> Result<()> {
478+ pub async fn update_pushed(&self, repo_id: &str, branch: &str, after: &str) -> Result<()> {
481479 let Some(row) = self.store.update_by_branch(repo_id, branch).await? else {
482480 return Ok(());
483481 };
482+ if row.state() == UpdateState::Superseded && row.pull().is_none() {
483+ // No longer needed by the time its sandbox pushed: the branch goes.
484+ return self.drop_pushed(repo_id, branch, after).await;
485+ }
484486 if row.state() != UpdateState::Requested {
485487 return Ok(());
486488 }
523525 return self.note(&row, "update_failed", Some(g1t_contracts::system::USERNAME), None, Some(&error)).await;
524526 }
525527 };
526− if let Some(older) = row.pull().filter(|older| *older != pull.number) {
527− self.close_with(&repo, older, format!("Superseded by #{}, which upgrades `{}` to {}.", pull.number, row.package, row.target))
528− .await?;
528+ if let Some(older) = row.pull().filter(|older| *older != pull.number)
529+ && let Some(found) = self.get_pull(&repo, older).await?
530+ && matches!(found.status, PullStatus::Open | PullStatus::Draft)
531+ && self
532+ .close_with(&repo, older, format!("Closed: superseded by #{}, which upgrades `{}` to {}.", pull.number, row.package, row.target))
533+ .await?
534+ && found.branch.as_deref() != Some(branch)
535+ {
536+ self.delete_pull_branch(&repo, &Pull { status: PullStatus::Closed, ..found }).await;
529537 }
530538 self.store.set_update(&row, UpdateState::Open, Some(pull.number), None, None).await?;
531539 // Labelled as the entry for its directory says, or `dependencies`
+10−0
769769 .results::<VulnRow>()
770770 }
771771
772+ /// Every vulnerability alert of a repository: open, fixed and dismissed.
773+ pub async fn all_vulnerabilities(&self, repo_id: &str) -> Result<Vec<VulnRow>> {
774+ self.db
775+ .prepare(format!("SELECT {VULN_COLUMNS} FROM vulnerabilities v {VULN_JOINS} WHERE v.repo_id = ?"))
776+ .bind(&[repo_id.into()])?
777+ .all()
778+ .await?
779+ .results::<VulnRow>()
780+ }
781+
772782 /// Replaces what is known about a repository's dependencies with what a
773783 /// scan found: new findings open, findings no longer true fixed, and
774784 /// findings that came back open again, or dismissed again when someone
+13−3
9595 }
9696
9797 /// A package name as its ecosystem compares names.
98−fn normalize(ecosystem: &str, name: &str) -> String {
98+pub(crate) fn normalize(ecosystem: &str, name: &str) -> String {
9999 if ecosystem == "pip" { manifests::python_name(name) } else { name.to_owned() }
100100 }
101101
899899 self.open_update_pull(&repo, &row, after).await?;
900900 Ok(true)
901901 }
902+ // No longer needed by the time its sandbox pushed: the branch goes.
903+ UpdateState::Superseded if row.pull().is_none() => {
904+ self.drop_pushed(repo_id, branch, after).await?;
905+ Ok(true)
906+ }
902907 _ => Ok(true),
903908 }
904909 }
978983 continue;
979984 }
980985 self.store.set_update_pull(&older.id, UpdateState::Superseded, older.pull(), None, None).await?;
981− if let Some(number) = older.pull() {
982− self.close_with(repo, number, format!("Superseded by #{}.", pull.number)).await?;
986+ if let Some(number) = older.pull()
987+ && let Some(found) = self.get_pull(repo, number).await?
988+ && matches!(found.status, PullStatus::Open | PullStatus::Draft)
989+ && self.close_with(repo, number, format!("Closed: superseded by #{}.", pull.number)).await?
990+ && older.branch != row.branch
991+ {
992+ self.delete_pull_branch(repo, &Pull { status: PullStatus::Closed, ..found }).await;
983993 }
984994 }
985995 // A grouped security update stands for each package's own.
+1−0
284284 &DeleteBranchArgs {
285285 repo_id: row.repo_id.clone(),
286286 branch: row.branch(),
287+ head: None,
287288 },
288289 )
289290 .await;