Skip to content

Commit

g1t closes its own update pull requests once they are no longer needed, and deletes their branches

A security update closes, with a comment saying why, once none of the alerts it fixes is open: fixed on the default branch (the vulnerable version left the lockfiles, e.g. after an overrides entry pushed to main) or dismissed. It is checked right after each dependency scan stores its findings, before anything else in the scan can fail, and when someone dismisses an alert. Grouped security updates close once every package's alerts are fixed or dismissed. A version update into the default branch closes once the lockfiles already have each dependency at its new version or later, or no longer have it. When an update pull request closes or merges, by g1t or a person, its branch is deleted while it is still at the pull request's last commit; each scan also removes branches left by update pull requests already closed. Superseded pull requests lose their branch too. repos' delete_branch accepts a branch outside g1t-* when the caller names the commit it must still point to, and never deletes the default branch.

syntaqxcommitted Parent1315c25Browse 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;
27742781 }
27752782 }
27762783
2784+/// Whether `delete_branch` may remove `branch`: one of g1t's own
2785+/// (`g1t-…`), or one whose tip the caller names, such as a dependency
2786+/// update's branch after its pull request closed.
2787+fn deletable_branch(branch: &str, head: Option<&str>) -> bool {
2788+ !branch.is_empty() && (branch.starts_with(G1T_BRANCH_PREFIX) || head.is_some_and(|head| !head.is_empty()))
2789+}
2790+
2791+#[cfg(test)]
2792+mod delete_branch_tests {
2793+ use super::deletable_branch;
2794+
2795+ #[test]
2796+ fn only_g1t_branches_or_a_named_tip_are_deleted() {
2797+ assert!(deletable_branch("g1t-queue-12", None));
2798+ assert!(!deletable_branch("g1t/security/sharp-0.35.5", None));
2799+ assert!(deletable_branch("g1t/security/sharp-0.35.5", Some("abc123")));
2800+ assert!(!deletable_branch("feature", Some("")));
2801+ assert!(!deletable_branch("", Some("abc123")));
2802+ }
2803+}
2804+
27772805 #[cfg(test)]
27782806 mod push_to_create_tests {
27792807 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;