From 41332f18fc65f509bc17f3d69b410a62c85b9c76 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 14:08:25 -0400 Subject: [PATCH 1/6] Pause Security runs after failed fixes and offer fixing (#544) --- CONTEXT.md | 4 +- README.md | 19 ++++- src/claim.rs | 9 ++- src/github/advisories.rs | 63 ++++++++++++++- src/notification.rs | 7 ++ src/pass.rs | 26 ++++++ src/run_ending.rs | 13 +++ src/security.rs | 158 ++++++++++++++++++++++++++++++++---- tests/security_run.rs | 169 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 442 insertions(+), 26 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index c28cca6..4bb74d5 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -69,7 +69,7 @@ One invocation of the factory on a single issue that is not a **Spec**, from lau A factory-owned checkout, on an **Issue branch** for a **Run** or detached for an **Architecture review**. Acquisition accounts for partial local effects, removing only attempt-owned clean artifacts on failure and retaining and naming work or uncertain artifacts. Failed-acquisition retention survives later Architecture review attempts. Successful acquisition carries checkout and registration identity through its lifetime; ordinary synchronization, observations and Failed run preservation share acquired-instance checks before and after every Git subprocess attempt, including retries, acting only on that acquired checkout and selected Issue branch and refusing changed or uncertain ownership. Failed run preservation retains work when ownership has changed or is uncertain and finishes after Command interruption without clearing it; ordinary synchronization remains interruptible. Confirmed Self-merge branch deletion uses the captured Launch repository independently of checkout validity. Cleanup checks stage-specific disposal authority before and after every destructive Git attempt, including retries: acquired identity and the captured cleanup head for checkout removal, removed-path absence and an unused expected head for local branch removal, then ref absence and the exact original local subsection for branch configuration removal. Only successful, verified transitions advance disposal; partial failures retain and name every remaining or uncertain resource. Live cleanup finishes after Command interruption without clearing it; stale disposal stays interruptible. Stale Architecture review scratch is disposable only with evidence of successful detached acquisition and unchanged ownership; unmarked or uncertain checkouts are retained. **Claim**: -The mark that the factory has taken an issue: thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that, when a **Run** or a **Spec run** starts on it, whether started on its **Issue URL** or by an **Architect run**, a **Pickup run** or a **Security run**. A **Ticket**'s Run in a Spec run and a **Base fix** make none. Ending a Claim finishes even after Command interruption. A Claim is released, `ready-for-agent` put back, only when the Run ends with nothing on `origin` to take over: no **Issue branch** and no pull request. Otherwise it stays while the issue is open, whether its pull request is ready for review or the Run failed, until the **Day shift** relabels it, and thirdshift takes the label off once the issue is closed. +The mark that the factory has taken an issue: thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that, when a **Run** or a **Spec run** starts on it, whether started on its **Issue URL** or by an **Architect run**, a **Pickup run** or a **Security run**. A **Ticket**'s Run in a Spec run and a **Base fix** make none. Ending a Claim finishes even after Command interruption. A failed Security fix keeps its Claim even with nothing on `origin`, so it stays with the Day shift. For other Runs, a Claim is released, `ready-for-agent` put back, only when the Run ends with nothing on `origin` to take over: no **Issue branch** and no pull request. Otherwise it stays while the issue is open, whether its pull request is ready for review or the Run failed, until the **Day shift** relabels it, and thirdshift takes the label off once the issue is closed. _Avoid_: lock (a lock is per machine and ends with its process), assignment **Spec run**: @@ -156,7 +156,7 @@ A **Run** asked to end with its pull request merged rather than left for review, _Avoid_: auto-merge (GitHub's own feature, which thirdshift does not use) **Run notification**: -A message thirdshift sends when a **Run**, a **Spec run**, an **Architect run** or a **Security run** ends, whatever its outcome (ready, merged, failed or interrupted; for an Architect run that dispatched nothing, plan published, idea filed, idea already filed or review failed), to the address given with the email flag or the default in the **User config**. Each sends one only when asked to, by the flag or by the User config; a Spec run's notification lists each **Ticket**'s outcome, and a Ticket's **Run** never sends one of its own. After an **Inherited failure** it carries, beside the cause, what the Run says on stderr: where the checks fail on the **Base branch**, and the offer of a **Base fix** if nobody decided against one. An Architect run's notification tells how its **Architecture review** ended, naming the plan or idea issue, and how the Spec run or Run it dispatched ended, which sends none of its own; a skipped one sends none. A **Pickup run** that took a **Ready issue** sends the one the Spec run or Run it dispatched would have sent, which sends none of its own; a skipped one sends none. A **Security run**'s notification lists each **Security finding**'s severity, title and private link, never its write-up, since the message passes through a third party; a skipped one sends none. Failing to send one never changes the outcome. +A message thirdshift sends when a **Run**, a **Spec run**, an **Architect run** or a **Security run** ends, whatever its outcome (ready, merged, failed or interrupted; for an Architect run that dispatched nothing, plan published, idea filed, idea already filed or review failed), to the address given with the email flag or the default in the **User config**. Each sends one only when asked to, by the flag or by the User config; a Spec run's notification lists each **Ticket**'s outcome, and a Ticket's **Run** never sends one of its own. After an **Inherited failure** it carries, beside the cause, what the Run says on stderr: where the checks fail on the **Base branch**, and the offer of a **Base fix** if nobody decided against one. An Architect run's notification tells how its **Architecture review** ended, naming the plan or idea issue, and how the Spec run or Run it dispatched ended, which sends none of its own; a skipped one sends none. A **Pickup run** that took a **Ready issue** sends the one the Spec run or Run it dispatched would have sent, which sends none of its own; a skipped one sends none. A **Security run**'s notification lists each **Security finding**'s severity, title and private link, never its write-up, since the message passes through a third party. When a Run leaves a reproduced finding unfixed and nobody decided against fixing, it offers both the command and the User config setting that allow fixing; a skipped one sends none. Failing to send one never changes the outcome. _Avoid_: completion email, alert **User config**: diff --git a/README.md b/README.md index 10fbec2..65b909f 100644 --- a/README.md +++ b/README.md @@ -748,15 +748,15 @@ The command takes the Harness, Model and Effort words and the shared Pass words Fixing is off by default. `security-fix` on the command or `fix = true` under `[security]` allows one fix; `no-security-fix` overrides the setting. Both words are accepted on every command that starts Runs and passed to those Runs. With fixing allowed, a Security run takes the most severe reproduced finding still awaiting a fix before auditing, with ties in private-record order. After an audit reproduces findings, it goes on to the most severe one. -A publishing session reads the private record and publishes one terse **Ticket**, labelled `needs-triage`, saying only what the fix changes and linking the record. thirdshift checks the Ticket, swaps `needs-triage` for `ready-for-agent`, adds `security-fix` (creating the label when missing), records the Ticket link privately and dispatches its **Run**. The Security run ends as that Run ends, including a **Merge run** when asked by the command or the User config. Every fix is a single Ticket for now; the write-up and proof-of-concept stay in the private record. +A publishing session reads the private record and publishes one terse **Ticket**, labelled `needs-triage`, saying only what the fix changes and linking the record. thirdshift checks the Ticket, swaps `needs-triage` for `ready-for-agent`, adds `security-fix` (creating the label when missing), records the Ticket link privately and dispatches its **Run**. The Security run ends as that Run ends, including a **Merge run** when asked by the command or the User config. Every fix is a single Ticket for now; the write-up and proof-of-concept stay in the private record. A failed fix keeps its **Claim**, even if it pushed nothing, and stays open for the **Day shift**. No later Security run or Pickup run retries it. Security runs pause while that fix's issue is open; closing the issue lets them go on. -`email`, optionally followed by an address, or `email.always` in the User config asks for one **Run notification** when the Security run ends, whether the audit succeeded, failed or was interrupted. `no-email` overrides the default. The notification says how the audit ended and lists each finding it recorded or matched to an existing private record: its severity when known, title and private link. It includes no finding write-up, trace or evidence, since it passes through Resend. Detailed failure causes stay in the local logs; the notification gives the audit's status and log paths. A recording failure still lists the records reached before it failed. A skipped Security run sends none. The usual address and Resend API key checks run before the skip checks or any work; a failed send is a warning and never changes the run's outcome. +`email`, optionally followed by an address, or `email.always` in the User config asks for one **Run notification** when the Security run ends, whether the audit succeeded, failed or was interrupted. `no-email` overrides the default. The notification says how the audit ended and lists each finding it recorded or matched to an existing private record: its severity when known, title and private link. It includes no finding write-up, trace or evidence, since it passes through Resend. Detailed failure causes stay in the local logs; the notification gives the audit's status and log paths. A recording failure still lists the records reached before it failed. When a run leaves a reproduced finding unfixed and neither the command nor the User config decided against fixing, its notification and stderr offer both `thirdshift secure security-fix` and `fix = true` under `[security]`. With `no-security-fix`, or the setting turned off, it offers nothing. A skipped Security run sends none. The usual address and Resend API key checks run before the skip checks or any work; a failed send is a warning and never changes the run's outcome. A Security run chooses its Harness from the command's `harness` word, then `[security] harness` in the User config, then `harness.default`, then Claude Code. A blank or missing Security setting keeps that default. Model and Effort come from the chosen Harness's own `[harness.]` section, with command words taking precedence. For example, `harness claude` overrides `[security] harness = "codex"` and uses `[harness.claude]`. Codex security sessions and their Resumes set `agents.max_concurrent_threads_per_session=8` and ask for fresh sub-agents with `fork_turns: "none"`, so the skill's verifiers stay independent. -A Security run checks its gates in order: another **Pass** on the repository running on the machine, a **Ready issue**, a **Security finding** waiting for the **Day shift**, then an unchanged **Base branch** since the last completed Security audit. An unreproduced draft advisory with no severity waits; closing it, publishing it or assigning severity ends that wait. A reproduced finding waits while fixing is disabled until its record is closed or published, or its fix Ticket is closed. On a private repository, an open finding issue with `needs-triage` waits until that label is removed or the issue is closed. The audited commit comes from the skill's own `run-metadata.json` under the audit root, with no separate state file. A skip exits `0`, starts no session, keeps no Command log and sends no Run notification; its reason goes into the repository's **Activity log** only when it changes, with the usual `activity.quiet_skips` behavior. Before starting work, thirdshift checks the chosen Harness and that **Node.js** is on `PATH`, since the embedded skill's report validators require it. +A Security run checks its gates in order: another **Pass** on the repository running on the machine, a **Ready issue**, a failed Security fix whose issue is still open, a **Security finding** waiting for the **Day shift**, then an unchanged **Base branch** since the last completed Security audit. An unreproduced draft advisory with no severity waits; closing it, publishing it or assigning severity ends that wait. A reproduced finding waits while fixing is disabled until its record is closed or published, or its fix Ticket is closed. On a private repository, an open finding issue with `needs-triage` waits until that label is removed or the issue is closed. The audited commit comes from the skill's own `run-metadata.json` under the audit root, with no separate state file. A skip exits `0`, starts no session, keeps no Command log and sends no Run notification; its reason goes into the repository's **Activity log** only when it changes, with the usual `activity.quiet_skips` behavior. Before starting work, thirdshift checks the chosen Harness and that **Node.js** is on `PATH`, since the embedded skill's report validators require it. The Security audit runs in a throwaway worktree detached at origin's Base branch head, leaving the Launch directory and local work untouched, including when `launch.pull` is set. Its Session prompt runs `thirdshift-security-audit` in full audit mode with the `quick` profile, auditing the whole repository except vendored and third-party code. It names a `SECURITY.md` or conventional threat-model document when present, reads compatible earlier runs and ends `incomplete` instead of asking for input. @@ -764,6 +764,19 @@ Each audit keeps a new output directory under `///audits/ A finding's title becomes the advisory summary. Its description preserves its write-up, trace, evidence and validation plan, with the fingerprint and audited commit. thirdshift claims no severity, CWE or affected version range. It uses the package in the repository's manifest, or the `other` ecosystem when none is identified. Matching fingerprints in any advisory state are kept rather than recorded again. Rejected candidates stay in the local report and produce no advisory. Draft advisories require GitHub repository security manager or administrator access. On a private repository whose advisory endpoint is unavailable, the private record is an issue labelled `security-finding` and `needs-triage`. It is never replaced by a public finding issue. +### Fencing + +**Fencing** is the factory finding and fixing vulnerabilities with nobody starting it: Security runs started on a schedule with fixing allowed. thirdshift never schedules itself ([ADR-0009](docs/adr/0009-the-operating-system-schedules-thirdshift.md)). This Linux crontab entry starts one every five minutes; use your own paths, with `claude` and `gh` already logged in and Git able to push: + +```cron +PATH=/home/you/.local/bin:/usr/local/bin:/usr/bin:/bin +*/5 * * * * cd /home/you/repos/widgets && thirdshift secure base main harness claude security-fix >> /home/you/thirdshift-secure.log 2>&1 +``` + +Each pass yields to a **Ready issue**, so work the **Day shift** shaped goes first. It is skipped while another Pass on the repository is running, while a fix has failed and its issue stays open, while a finding waits for triage and no reproduced finding can be fixed, or while the Base branch is unchanged and no fix is waiting. A failed fix pauses Fencing for the Day shift until its issue is closed; it is never retried by a later Security run or Pickup run. Triaging a finding lets audits go on; fixing allowed can still take a reproduced finding ahead of a different finding waiting for triage. + +A skip exits `0` and sends no Run notification. Its reason appears in the **Activity log** once per change of reason. Set `activity.quiet_skips = true` in the [User config](#user-config) to keep those skips out of the scheduler's log. `merge` on the command, or `merge.always = true` in the User config, lets each fix's Run merge its pull request; otherwise it stays ready for review. + ## Pickup runs `thirdshift pickup` starts a **Pickup run**: one pass, with no **Issue URL**, that takes the lowest-numbered **Ready issue** in the repository and runs it, so that an issue you have already marked ready needs no command typed for it. Start it from a clone of the repository, on the **Base branch**, or name the Base branch with `base `: diff --git a/src/claim.rs b/src/claim.rs index 351eb0e..050e627 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -3,7 +3,8 @@ //! Run or a Spec run is working on. It is made before the run's work, and //! ended once with how the run ended: removed once its Self-merge has left the //! issue closed, kept while its pull request waits for review, and released -//! when the run failed with nothing on origin to take over. +//! when the run failed with nothing on origin to take over, except a failed +//! Security fix, which stays with the Day shift. //! //! What it reads and changes of GitHub and origin, whether the run is //! interrupted, and its progress lines all go through [`Outside`]: @@ -63,7 +64,7 @@ impl Claim<'_> { /// closed. /// - Ready for review: the Claim stays while the pull request waits. /// - Failed: the Claim is released if nothing of the run is on origin to - /// take over. + /// take over, except a Security fix, whose Claim stays. /// /// The run has ended as it has, so this never fails: a failure is a /// warning naming what to run by hand, and an interrupt doesn't stop it. @@ -156,6 +157,8 @@ struct Claimed<'a> { added_in_progress: bool, /// Whether making it took `ready-for-agent` off the issue. removed_ready_for_agent: bool, + /// A failed Security fix stays with the Day shift even without a push. + security_fix: bool, } impl<'a> Claimed<'a> { @@ -177,6 +180,7 @@ impl<'a> Claimed<'a> { issue, added_in_progress: edit.puts_on(IN_PROGRESS), removed_ready_for_agent: edit.takes_off(READY_FOR_AGENT), + security_fix: edit.labels().has(crate::security::fixing::SECURITY_FIX), }; if !claimed.changed_any() { return Ok(claimed); @@ -198,6 +202,7 @@ impl<'a> Claimed<'a> { match reached { Some(Goal::Merged) => self.remove_if_closed(outside), Some(Goal::ReadyForReview) => {} + None if self.security_fix => {} None => self.release_if_nothing_on_origin(outside), } } diff --git a/src/github/advisories.rs b/src/github/advisories.rs index 44a954a..ffd2b44 100644 --- a/src/github/advisories.rs +++ b/src/github/advisories.rs @@ -17,6 +17,8 @@ const SECURITY_FINDING: Label = Label::new( pub(crate) const FIX_TICKET_MARKER: &str = "\n\nFix Ticket: "; +const FIX_FAILED: &str = "Fix Run: failed"; + pub enum SecurityRecords { Advisories(Vec), Issues(Vec), @@ -53,6 +55,29 @@ impl SecurityRecords { } } + /// A dispatched fix that failed and whose issue the Day shift has not closed. + pub fn failed_fix(&self) -> Result> { + let (records, field) = match self { + Self::Advisories(records) => (records, "description"), + Self::Issues(records) => (records, "body"), + }; + for record in records { + if record["security_fix_closed"] == true { + continue; + } + if let Some((_, fix)) = record[field] + .as_str() + .and_then(|text| text.rsplit_once(FIX_TICKET_MARKER)) + && fix.lines().any(|line| line == FIX_FAILED) + { + return Ok(Some(IssueUrl::parse( + fix.lines().next().unwrap_or_default(), + )?)); + } + } + Ok(None) + } + /// Most severe reproduced finding still awaiting a fix, ties in record order. /// A recorded Ticket has already been dispatched; its Run owns that fix. pub fn next_fix(&self) -> Result> { @@ -224,6 +249,26 @@ impl GitHub { repo: &str, record: &SecurityRecord, ticket: &IssueUrl, + ) -> Result<()> { + self.write_security_fix(repo, record, ticket, false) + } + + /// Preserve a failed fix's ending in its private record for later Passes. + pub fn record_failed_security_fix( + &self, + repo: &str, + record: &SecurityRecord, + ticket: &IssueUrl, + ) -> Result<()> { + self.write_security_fix(repo, record, ticket, true) + } + + fn write_security_fix( + &self, + repo: &str, + record: &SecurityRecord, + ticket: &IssueUrl, + failed: bool, ) -> Result<()> { let (path, field) = match record { SecurityRecord::Advisory { id, .. } => ( @@ -243,14 +288,24 @@ impl GitHub { } let current: Value = serde_json::from_slice(&output.stdout).context("private record is invalid JSON")?; - if current[field].as_str() != Some(record.description()) { - bail!("the private record changed while publishing its fix; leaving it unchanged"); - } - let description = format!( + let linked = format!( "{}{FIX_TICKET_MARKER}{}\n", record.description().trim_end(), ticket.url ); + let expected = if failed { + linked.as_str() + } else { + record.description() + }; + if current[field].as_str() != Some(expected) { + bail!("the private record changed while publishing its fix; leaving it unchanged"); + } + let description = if failed { + format!("{linked}{FIX_FAILED}\n") + } else { + linked + }; let body = serde_json::to_vec(&json!({field: description}))?; let output = self.output_with_input( &["api", "--method", "PATCH", &path, "--input", "-"], diff --git a/src/notification.rs b/src/notification.rs index 4e908bf..dbcd200 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -252,6 +252,11 @@ fn body( text += &format!("Cause: {cause}\n"); } } + if let Some(offer) = account.security_fix_offer { + for line in offer.lines() { + text += &format!("{line}\n"); + } + } for line in account.advice { text += &format!("{:<14}{}\n", format!("{}:", line.label), line.value); } @@ -331,6 +336,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + security_fix_offer: None, urls: vec![PR], } } @@ -349,6 +355,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + security_fix_offer: None, urls: Vec::new(), } } diff --git a/src/pass.rs b/src/pass.rs index 21732ee..e69ced4 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -137,6 +137,12 @@ pub trait Outside { url: &str, ) -> (Result, Option); fn link_security_fix(&mut self, record: &SecurityRecord, ticket: &IssueUrl) -> Result<()>; + /// Keep a failed dispatch in its private record, even after interruption. + fn record_failed_security_fix( + &mut self, + record: &SecurityRecord, + ticket: &IssueUrl, + ) -> Result<()>; /// Run `dispatch` to its end, on the Harness the pass checked. fn dispatch(&mut self, dispatch: Dispatch) -> Ended; } @@ -325,6 +331,16 @@ impl Outside for LaunchAndGitHub<'_> { fn link_security_fix(&mut self, record: &SecurityRecord, ticket: &IssueUrl) -> Result<()> { GitHub::new().link_security_fix(&self.repo.slug(), record, ticket) } + + fn record_failed_security_fix( + &mut self, + record: &SecurityRecord, + ticket: &IssueUrl, + ) -> Result<()> { + GitHub::new() + .completion() + .record_failed_security_fix(&self.repo.slug(), record, ticket) + } } #[cfg(test)] @@ -389,6 +405,7 @@ mod in_memory { UpdateSecurityRecord(String), PublishSecurityFix(String), LinkSecurityFix(u64), + FailedSecurityFix(u64), DispatchSecurityFix { ticket: u64, base: String, @@ -838,6 +855,15 @@ mod in_memory { self.calls.push(Call::LinkSecurityFix(ticket.number)); Ok(()) } + + fn record_failed_security_fix( + &mut self, + _record: &SecurityRecord, + ticket: &IssueUrl, + ) -> Result<()> { + self.calls.push(Call::FailedSecurityFix(ticket.number)); + Ok(()) + } } /// Issue `number` in `acme/widgets` as a listing gives it, labelled diff --git a/src/run_ending.rs b/src/run_ending.rs index 2683120..17248ba 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -53,12 +53,14 @@ pub fn read(ending: &Ending) -> Result, &Skip> { ticket_lines: &[], review: None, security_findings: None, + security_fix_offer: None, urls: Vec::new(), }, Err(failed) => Account::of_failure(failed, "audit failed"), }; Ok(Account { security_findings: Some(&audited.findings), + security_fix_offer: audited.offer.as_ref(), ..account }) } @@ -103,6 +105,8 @@ pub struct Account<'a> { pub review: Option>, /// Safe metadata from a Security run's private records, even if the audit failed. pub security_findings: Option<&'a [crate::security::RecordedFinding]>, + /// An offer to allow fixing when no command or setting decided against it. + pub security_fix_offer: Option<&'a crate::security::FixOffer>, /// The URLs on stdout, a line each: the pull request's, or that of the /// issue an Architect run's review ended on. pub urls: Vec<&'a str>, @@ -171,6 +175,7 @@ impl<'a> Account<'a> { ticket_lines: &reached.ticket_lines, review: None, security_findings: None, + security_fix_offer: None, urls: vec![&reached.pr_url], }, Err(failed) => Account { @@ -195,6 +200,7 @@ impl<'a> Account<'a> { ticket_lines: &failed.ticket_lines, review: None, security_findings: None, + security_fix_offer: None, urls: failed.pr_url.as_deref().into_iter().collect(), } } @@ -223,6 +229,7 @@ impl<'a> Account<'a> { ticket_lines: &[], review: None, security_findings: None, + security_fix_offer: None, urls: vec![reviewed.url()], }, (Err(failed), None) => Account::of_failure(failed, "review failed"), @@ -305,6 +312,9 @@ impl Shown { } } } + if let Some(offer) = account.security_fix_offer { + steps.extend(offer.lines()); + } Shown { steps, urls: account.urls.iter().map(|url| url.to_string()).collect(), @@ -476,6 +486,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + security_fix_offer: None, urls: vec![PR], } } @@ -494,6 +505,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + security_fix_offer: None, urls: Vec::new(), } } @@ -670,6 +682,7 @@ mod tests { dispatched: None, }), security_findings: None, + security_fix_offer: None, urls: vec![PLAN], } ); diff --git a/src/security.rs b/src/security.rs index 5e1a1ee..622805b 100644 --- a/src/security.rs +++ b/src/security.rs @@ -39,6 +39,7 @@ pub enum Outcome { pub struct Ended { pub outcome: Result, pub findings: Vec, + pub offer: Option, } impl From for Ended { @@ -46,10 +47,45 @@ impl From for Ended { Self { outcome: Err(error.into()), findings: Vec::new(), + offer: None, } } } +/// The two ways an undecided operator can allow fixing, shared by stderr and email. +#[derive(Debug, PartialEq, Eq)] +pub struct FixOffer { + command: String, +} + +impl FixOffer { + pub fn lines(&self) -> [String; 2] { + [ + format!("Allow fixing: {}", self.command), + "Or set: fix = true under [security] in ~/.thirdshift/config.toml".to_string(), + ] + } +} + +/// Preserve the typed command, quoting its words so the offered command can be run. +fn command_with_fixing() -> String { + let words = std::env::args() + .skip(1) + .map(|word| { + if word + .chars() + .all(|c| c.is_ascii_alphanumeric() || "-_/.:@=".contains(c)) + && !word.is_empty() + { + word + } else { + format!("'{}'", word.replace('\'', "'\\''")) + } + }) + .collect::>(); + format!("thirdshift {} security-fix", words.join(" ")) +} + /// Only the metadata allowed in a Run notification; no private write-up. #[derive(Debug, PartialEq, Eq)] pub struct RecordedFinding { @@ -79,6 +115,7 @@ pub enum Skipped { AlreadyRunning(AlreadyRunning), ReadyIssue(ListedIssue), FindingWaiting, + FailedFix(crate::issue::IssueUrl), UnchangedBase(String), } @@ -91,6 +128,11 @@ impl fmt::Display for Skipped { "Ready issue #{} \"{}\" goes first: {}", listed.issue.number, listed.title, listed.issue.url ), + Self::FailedFix(ticket) => write!( + f, + "failed Security fix #{} is still open for the Day shift: {}", + ticket.number, ticket.url + ), Self::FindingWaiting => f.write_str("a Security finding is waiting for the Day shift"), Self::UnchangedBase(base) => write!( f, @@ -133,6 +175,8 @@ pub fn run( } Err(error) => return Outcome::Audited(error.into()), }; + let offer_command = + (flags.security_fix.is_none() && config.security_fix.is_none()).then(command_with_fixing); run_through( &mut LaunchAndGitHub { launch: &directory, @@ -145,10 +189,17 @@ pub fn run( &repo, base.name(), flags.security_fix_allowed(config), + offer_command.as_deref(), ) } -fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str, fixing: bool) -> Outcome { +fn run_through( + outside: &mut impl Outside, + repo: &Repo, + base: &str, + fixing: bool, + offer_command: Option<&str>, +) -> Outcome { match outside.ready_issue() { Ok(Some(ready)) => { let skipped = Skipped::ReadyIssue(ready.listed); @@ -162,6 +213,15 @@ fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str, fixing: bool Ok(records) => records, Err(error) => return Outcome::Audited(error.into()), }; + match records.failed_fix() { + Ok(Some(ticket)) => { + let skipped = Skipped::FailedFix(ticket); + outside.skipped(Pass::Security, &skipped); + return Outcome::Skipped(skipped); + } + Ok(None) => {} + Err(error) => return Outcome::Audited(error.into()), + } if fixing { match records.next_fix() { Ok(Some((record, metadata))) => return fix(outside, repo, base, record, metadata), @@ -183,7 +243,7 @@ fn run_through(outside: &mut impl Outside, repo: &Repo, base: &str, fixing: bool Ok(false) => {} Err(error) => return Outcome::Audited(error.into()), } - let audited = audit_and_record(outside, repo, base); + let audited = audit_and_record(outside, repo, base, offer_command); if fixing && audited.outcome.is_ok() { match outside .security_records() @@ -226,10 +286,17 @@ fn fix( log = session_log; let ticket = published?; outside.link_security_fix(&record, &ticket)?; - Ok(outside.dispatch(crate::pass::Dispatch::SecurityFix { + let mut ended = outside.dispatch(crate::pass::Dispatch::SecurityFix { ticket: &ticket, base, - })) + }); + if let Err(failed) = &mut ended.outcome + && let Err(error) = outside.record_failed_security_fix(&record, &ticket) + { + let cause = format!("could not record the failed Security fix's ending: {error:#}"); + failed.error = std::mem::replace(&mut failed.error, error).context(cause); + } + Ok(ended) })(); Outcome::Fixed { ended: ended.unwrap_or_else(|error| crate::run::Ended { @@ -244,7 +311,13 @@ fn fix( } } -fn audit_and_record(outside: &mut impl Outside, repo: &Repo, base: &str) -> Ended { +fn audit_and_record( + outside: &mut impl Outside, + repo: &Repo, + base: &str, + offer_command: Option<&str>, +) -> Ended { + let mut offer = None; let mut findings = Vec::new(); let mut log = None; let recorded = (|| -> Result { @@ -300,6 +373,11 @@ fn audit_and_record(outside: &mut impl Outside, repo: &Repo, base: &str) -> Ende } let reproduced = reproduced?; outside.update_security_record(record, &reproduced)?; + if reproduced.severity().is_some() { + offer = offer_command.map(|command| FixOffer { + command: command.to_string(), + }); + } for finding in findings.iter_mut().filter(|finding| finding.url == *url) { finding.severity = reproduced .severity() @@ -318,6 +396,7 @@ fn audit_and_record(outside: &mut impl Outside, repo: &Repo, base: &str) -> Ende ..error.into() }), findings, + offer, } } @@ -345,7 +424,7 @@ mod tests { if ready { outside = outside.ready(7, false); } - let outcome = run_through(&mut outside, &widgets(), "main", true); + let outcome = run_through(&mut outside, &widgets(), "main", true, None); if ready { assert!(matches!(outcome, Outcome::Skipped(Skipped::ReadyIssue(_)))); assert!(matches!( @@ -383,7 +462,7 @@ mod tests { "ghsa_id": "GHSA-test", "summary": "Unchecked input size" })]) .harness_failing(); - let outcome = run_through(&mut outside, &widgets(), "main", false); + let outcome = run_through(&mut outside, &widgets(), "main", false, None); assert!(matches!(outcome, Outcome::Skipped(_))); assert_eq!( outside.calls, @@ -408,7 +487,7 @@ mod tests { let mut outside = InMemory::default().advisories(vec![serde_json::json!({ "state": state, "severity": severity })]); - let outcome = run_through(&mut outside, &widgets(), "main", false); + let outcome = run_through(&mut outside, &widgets(), "main", false, None); assert_eq!( matches!(outcome, Outcome::Skipped(_)), waits, @@ -434,7 +513,7 @@ mod tests { "state": state, "labels": labels.iter().map(|name| serde_json::json!({"name":name})).collect::>() })]); - let outcome = run_through(&mut outside, &widgets(), "main", false); + let outcome = run_through(&mut outside, &widgets(), "main", false, None); assert_eq!( matches!(outcome, Outcome::Skipped(_)), waits, @@ -450,7 +529,7 @@ mod tests { #[test] fn checks_prerequisites_then_audits_without_pulling() { let mut outside = InMemory::default(); - let outcome = run_through(&mut outside, &widgets(), "main", false); + let outcome = run_through(&mut outside, &widgets(), "main", false, None); assert!(matches!( outcome, Outcome::Audited(Ended { outcome: Ok(_), .. }) @@ -477,7 +556,7 @@ mod tests { .node_failing() .harness_failing(); assert!(matches!( - run_through(&mut outside, &widgets(), "main", false), + run_through(&mut outside, &widgets(), "main", false, None), Outcome::Skipped(Skipped::ReadyIssue(_)) )); assert!(matches!( @@ -503,7 +582,7 @@ mod tests { if unchanged { outside = outside.unchanged_base(); } - let outcome = run_through(&mut outside, &widgets(), "main", false); + let outcome = run_through(&mut outside, &widgets(), "main", false, None); let expected = if ready { "Ready issue #7" } else if waiting { @@ -533,6 +612,55 @@ mod tests { } } + #[test] + fn a_failed_fix_yields_to_ready_work_but_precedes_waiting_and_audit_history() { + for private in [false, true] { + for ready in [false, true] { + for closed in [false, true] { + for fixing in [false, true] { + let body = "Private record\n\nFix Ticket: https://github.com/acme/widgets/issues/8\nFix Run: failed\n"; + let values = vec![ + serde_json::json!({ + "state": if private { "open" } else { "draft" }, + "severity": null, "description": body, "body": body, + "security_fix_closed": closed, + }), + serde_json::json!({ + "state": if private { "open" } else { "draft" }, + "severity": null, "labels": [{"name":"needs-triage"}], + }), + ]; + let mut outside = if private { + InMemory::default().finding_issues(values) + } else { + InMemory::default().advisories(values) + } + .unchanged_base(); + if ready { + outside = outside.ready(7, false); + } + let Outcome::Skipped(skipped) = + run_through(&mut outside, &widgets(), "main", fixing, None) + else { + panic!("Security run should skip"); + }; + let expected = if ready { + "Ready issue #7" + } else if !closed { + "failed Security fix #8 is still open" + } else { + "a Security finding is waiting for the Day shift" + }; + assert!(skipped.to_string().starts_with(expected), "{skipped}"); + assert!(!outside.calls.contains(&Call::HarnessCheck)); + assert!(!outside.calls.contains(&Call::AuditHistory)); + assert_eq!(outside.calls.contains(&Call::AdvisoryList), !ready); + } + } + } + } + } + #[test] fn each_failed_prerequisite_prevents_later_operations() { for (mut outside, expected) in [ @@ -557,7 +685,7 @@ mod tests { ), ] { assert!(matches!( - run_through(&mut outside, &widgets(), "main", false), + run_through(&mut outside, &widgets(), "main", false, None), Outcome::Audited(Ended { outcome: Err(_), .. @@ -575,7 +703,7 @@ mod tests { let Outcome::Audited(Ended { outcome: Err(failed), .. - }) = run_through(&mut outside, &widgets(), "main", false) + }) = run_through(&mut outside, &widgets(), "main", false, None) else { panic!("audit should fail"); }; @@ -617,7 +745,7 @@ mod tests { let Outcome::Audited(Ended { outcome: Ok(recorded), .. - }) = run_through(&mut outside, &widgets(), "main", false) + }) = run_through(&mut outside, &widgets(), "main", false, None) else { panic!("audit should succeed"); }; diff --git a/tests/security_run.rs b/tests/security_run.rs index 043fb97..dd6f578 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -566,6 +566,175 @@ fn the_security_run_ends_as_a_failed_fix_and_sends_one_private_metadata_notifica assert!(!body.contains("bounded_fixture")); } +#[test] +fn a_failed_fix_keeps_its_claim_and_pauses_security_and_pickup_until_closed() { + for private in [false, true] { + for pushed in [false, true] { + let scenario = with_reproduced_findings(&["critical"]); + if private { + let mut state = scenario.gh_state(); + state["private"] = json!(true); + state["bodies"]["7"] = state["advisories"][0]["description"].clone(); + state["labels"]["7"] = json!(["security-finding", "needs-triage"]); + scenario.write_gh_state(&state); + } + let publishing = publish_fix("GHSA-finding-0"); + scenario.agent_does_in_session( + 1, + &if private { + publishing.replace("security/advisories/GHSA-finding-0", "issues/7") + } else { + publishing + }, + ); + scenario.agent_does_for( + 8, + &format!("{}exit 1\n", if pushed { implement_fix() } else { "" }), + ); + let failed = scenario.run(&["secure", "security-fix"]); + assert_eq!(failed.code, Some(1), "{}", failed.stderr); + assert_eq!( + scenario.gh_state()["labels"]["8"], + json!(["security-fix", "in-progress"]) + ); + // Even contradictory ready labelling cannot make Pickup retry a Claim. + scenario.issue_labelled(8, &["security-fix", "in-progress", "ready-for-agent"]); + let resend = ResendStandIn::replying(200, r#"{"id":"sent"}"#); + for args in [ + vec!["secure", "security-fix", "email", "day@example.com"], + vec!["secure", "no-security-fix", "email", "day@example.com"], + vec!["pickup"], + ] { + let skipped = scenario.run_with_env(&args, &resend_env(&resend)); + assert_eq!(skipped.code, Some(0), "{}", skipped.stderr); + assert!(skipped.stdout.is_empty()); + if args[0] == "secure" { + assert!( + skipped + .stderr + .contains("failed Security fix #8 is still open"), + "{}", + skipped.stderr + ); + } + } + assert_eq!(scenario.claude_calls().len(), 2); + assert!(resend.requests().is_empty()); + assert_eq!( + scenario + .entries("home/.thirdshift/logs/acme/widgets/commands/secure") + .len(), + 1 + ); + let activity = fs::read_to_string( + scenario.path("home/.thirdshift/logs/acme/widgets/activity.log"), + ) + .unwrap(); + assert!( + activity + .matches("Security run skipped: failed Security fix #8 is still open") + .count() + == 1, + "{activity}" + ); + let mut state = scenario.gh_state(); + state["issues"]["8"] = json!("CLOSED"); + scenario.write_gh_state(&state); + scenario.agent_does(&audit_script("[]")); + let after = scenario.run(&["secure", "security-fix"]); + assert_eq!(after.code, Some(0), "{}", after.stderr); + assert!( + after + .stderr + .contains("Security audit recorded 0 new finding(s)"), + "{}", + after.stderr + ); + assert_eq!(scenario.claude_calls().len(), 3); + } + } +} + +#[test] +fn a_successful_fix_awaiting_review_does_not_pause_allowed_security_work() { + let scenario = with_reproduced_findings(&["high"]); + scenario.agent_does_in_session(1, &publish_fix("GHSA-finding-0")); + scenario.agent_does_for(8, implement_fix()); + let fixed = scenario.run(&["secure", "security-fix"]); + assert_eq!(fixed.code, Some(0), "{}", fixed.stderr); + scenario.agent_does(&audit_script("[]")); + let audited = scenario.run(&["secure", "security-fix"]); + assert_eq!(audited.code, Some(0), "{}", audited.stderr); + assert!( + audited + .stderr + .contains("Security audit recorded 0 new finding(s)"), + "{}", + audited.stderr + ); + assert_eq!(scenario.claude_calls().len(), 3); +} + +#[test] +fn an_undecided_run_offers_both_ways_to_allow_a_reproduced_fix() { + for (config, permission, reproduced, offers) in [ + ("", None, true, true), + ("", Some("no-security-fix"), true, false), + ("[security]\nfix = false\n", None, true, false), + ( + "[security]\nfix = true\n", + Some("no-security-fix"), + true, + false, + ), + ("", None, false, false), + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + scenario.agent_does_in_session(1, &audit_script(&json!([finding("offer")]).to_string())); + scenario.agent_does_in_session( + 2, + &reproduction_script(if reproduced { + "reproduced high single" + } else { + "not reproduced" + }), + ); + let resend = ResendStandIn::replying(200, r#"{"id":"sent"}"#); + let mut args = vec![ + "secure", + "base", + "main", + "harness", + "claude", + "email", + "day@example.com", + ]; + if let Some(permission) = permission { + args.push(permission); + } + let result = scenario.run_with_env(&args, &resend_env(&resend)); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let (_, body) = the_one_notification(&resend); + for text in [&result.stderr, &body] { + assert_eq!( + text.contains( + "thirdshift secure base main harness claude email day@example.com security-fix" + ), + offers, + "{text}" + ); + assert_eq!( + text.contains("fix = true under [security]"), + offers, + "{text}" + ); + assert!(!text.contains("Private candidate write-up"), "{text}"); + } + assert_eq!(scenario.claude_calls().len(), 2); + } +} + #[test] fn invalid_fix_tickets_are_not_marked_ready_or_dispatched() { for script in [ From 928bb0fdb9011b861142ca2a4ef45e8b3aa44737 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 14:18:46 -0400 Subject: [PATCH 2/6] Keep failed fixes paused through GitHub errors and address review findings --- CONTEXT.md | 2 +- README.md | 2 +- src/base_fix.rs | 9 +- src/github/advisories.rs | 101 ++++++++++++---------- src/main.rs | 13 ++- src/notification.rs | 7 -- src/pass.rs | 30 +++++-- src/ready.rs | 10 ++- src/run_ending.rs | 35 +++----- src/security.rs | 64 ++++++++------ tests/security_run.rs | 176 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 328 insertions(+), 121 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 4bb74d5..236f28f 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -109,7 +109,7 @@ A **Pickup run**'s removal of `in-progress` from every closed issue in the repos _Avoid_: cleanup, garbage collection **Ready issue**: -An open issue a **Pickup run** may take: labelled `ready-for-agent`, with no label that makes an **Unready Ticket** and no **Claim**, not a sub-issue, not a **Base fix**'s issue, with no open blocker, never started (no **Issue branch** and no pull request), not a **Spec** whose **Tickets** are all closed, and left untouched long enough that whoever is shaping it has finished. On a Spec, the label says its Tickets are published. A `ready-for-agent` Ticket inside a Spec is not one: it is reached through its Spec, when the Spec is itself a Ready issue. +An open issue a **Pickup run** may take: labelled `ready-for-agent`, with no label that makes an **Unready Ticket** and no **Claim**, not a sub-issue, not a **Base fix**'s or a **Security run**'s fix issue, with no open blocker, never started (no **Issue branch** and no pull request), not a **Spec** whose **Tickets** are all closed, and left untouched long enough that whoever is shaping it has finished. On a Spec, the label says its Tickets are published. A `ready-for-agent` Ticket inside a Spec is not one: it is reached through its Spec, when the Spec is itself a Ready issue. _Avoid_: queued issue, backlog item **Architecture review**: diff --git a/README.md b/README.md index 65b909f..8a6fe59 100644 --- a/README.md +++ b/README.md @@ -794,7 +794,7 @@ A Ready issue is an open issue that: - has none of the labels that make an **Unready Ticket**, `ready-for-human`, `needs-info`, `wontfix` and `needs-triage`, so a contradictory label errs on the side of not running; - carries no [Claim](#the-claim): it is not labelled `in-progress`; - is not a sub-issue. A sub-issue is a **Ticket** of a **Spec**, and is never run on its own, whatever the Spec's labels: it is reached through its Spec, when the Spec is itself a Ready issue, so its work always goes through the **Spec branch**. Labelling one Ticket never promotes its Spec, either; -- is not labelled `base-fix`: the Run that opened a [Base fix](#base-fix)'s issue owns it; +- is not labelled `base-fix` or `security-fix`: the Run that opened a [Base fix](#base-fix)'s issue, or the Security run that dispatched a fix, owns it. A failed Security fix is still passed over if GitHub prevented its Claim from being made; - has no open blocker, by GitHub's "blocked by" links, never the text of its body. Once every blocker is closed, it can be taken; - was never started: no **Issue branch** for it is on `origin`, and no pull request from one exists, open, merged or closed. So a Pickup run never does a [Continuation](#continuation), and an issue whose Run failed waits for you; - is not a **Spec** whose **Tickets** are all closed. With nothing started on it, such a Spec was done some other way, and the [Spec run](#spec-runs) it would be dispatched as stops with nothing to do, so a Pickup run passes it over rather than take it on every pass. Waiting never makes it a Ready issue, so this comes before the next condition, however lately the Spec was labelled: close it, or take its `ready-for-agent` off. A Spec with at least one open Ticket is taken; diff --git a/src/base_fix.rs b/src/base_fix.rs index 78d1e27..82ac205 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -61,11 +61,10 @@ const RETRY_WITH: &str = "Retry with"; /// What starts the line of [`Advice`] naming the User config's `base.fix`. const OR_SET: &str = "Or set"; -/// A line of the advice a Run gives after its cause when Inherited failures -/// fail it with no Base fix taken: each check where it fails on the Base -/// branch, then, if nobody decided against a Base fix, how to allow one. It -/// reads `