patchwork: enhance integration

[derekbarbosa]

Patchwork Policy Modes: API and Email-Based Check Updates

Summary

This PR adds dual-mode patchwork integration to sashiko, allowing
subsystem maintainers to report review results as patchwork checks
via either direct API calls or structured email notifications.

Motivation

Patchwork's permission model is coarse-grained -- an API token that
can post checks can also change patch states, delegate patches, and
perform other maintainer operations. Some subsystems (like linux-media)
are not comfortable giving sashiko a write token with that scope.

This PR provides two delivery modes so each subsystem can choose the
integration that fits their security posture:

• API mode for subsystems that accept the token risk in exchange
  for direct, low-latency check delivery
• Email mode for subsystems that prefer sashiko to send a
  structured email that a local script (like pw_tools) parses and
  posts using the maintainer's own credentials

What changed

Config (email_policy.rs):

• New patchwork.email field for email-based mode
• New patchwork.min_severity for per-subsystem severity filtering
• New patchwork.fail_severity (default: "High") for configurable
  check state mapping: new findings at or above threshold produce
  fail, below produce warning, pre-existing findings never
  affect state
• SASHIKO_PATCHWORK_TOKEN env var fills in tokens where the TOML
  omits them (explicit TOML tokens take precedence)
• URL normalization on load: trailing slash stripping, scheme
  validation, non-localhost http:// security warning

Schema (schema.sql):

• New patchwork_outbox table with status index and next_retry_at
  column for non-blocking retry scheduling
• No secrets stored in the database -- tokens are resolved from
  the config file at delivery time

Runtime (patchwork.rs, worker/patchwork.rs, reviewer.rs):

• PatchworkCheckResult::from_policy() builder computes check state
  and per-severity description from policy config and findings,
  keeping PatchworkPolicy as a pure config struct
• Check description shows per-severity breakdown with pre-existing
  counts in parentheses, dropping zero-count severities (e.g.
  Critical: 1 · High: 2 (1 pre-existing))
• PatchworkWorker polls the outbox, resolves the API token from
  the email policy config by matching api_url, posts checks,
  retries on failure with non-blocking exponential backoff via
  next_retry_at timestamps (5s/30s/180s), reclaims ghost entries
  after 10 minutes, uses max_retries from settings
• Reviewer inserts into the appropriate outbox(es) instead of
  fire-and-forget tokio::spawn
• post_patchwork_check() refactored to return Result, accept a
  shared reqwest::Client, URL-encode message-IDs, and warn on
  ambiguous multi-patch results

Bugfix (db.rs):

• EmailOutboxRow.patch_id changed from i64 to Option<i64> to
  match the schema which allows NULL. Required for patchwork email
  notifications that use patch_id = NULL to avoid the dedup guard.

Docs (configuration.md, examples/email_policy.toml):

• Full patchwork integration section covering both modes, severity
  filtering, check state mapping, edge case behaviors, token env
  var, and structured email format specification

Patchwork API interaction

Two endpoints, executed sequentially by the PatchworkWorker:

1. GET {api_url}/patches/?msgid={encoded_msgid} -- resolve the
   email message-ID to a patchwork patch ID
2. POST {api_url}/patches/{patch_id}/checks/ -- submit the check
   result (state, description, target_url, context)

Email mode skips both endpoints. It sends a parseable plain-text
email that a downstream tool converts into these same API calls
using the maintainer's own token.

Design tradeoffs

Database-backed outbox with polling worker over inline retry in
tokio::spawn. The outbox survives crashes; ghost sweep recovers stuck
entries; the table is queryable for ops visibility.

Non-blocking retry via next_retry_at timestamps over sleeping in
the worker loop. Failed entries are immediately returned to Pending
with a future timestamp. The worker skips them until the backoff
expires and continues processing other entries.

Tokens resolved from config at delivery time over storing tokens
in the outbox table. The PatchworkWorker loads the email policy config
and matches by api_url to resolve the correct token. No secrets in
the database. Token rotation takes effect immediately.

PatchworkCheckResult builder pattern over inline computation in
reviewer.rs. Separates config (PatchworkPolicy) from computed output
(state + description). Keeps the policy struct as pure deserialized
config.

Check state from new findings only with configurable threshold.
Pre-existing findings are shown in the description for context but
never trigger fail or warning. This prevents noise from
pre-existing technical debt affecting the patch review signal.

Separate patchwork_outbox table over extending email_outbox.
Different semantics (retry with backoff vs one-shot), different
columns, cleaner separation.

Email mode reuses email_outbox with NULL patch_id over a separate
table. Reuses the existing EmailWorker and SMTP infrastructure.

Env var fills token gaps, does not overwrite over always
overriding TOML. Preserves per-subsystem token differentiation.

Known limitations

• No graceful worker shutdown -- pre-existing across all workers.
  Ghost sweep recovers in-flight entries after 10 minutes. Patchwork
  dedups by context so duplicate check posts are harmless.
• No SSRF protection on api_url -- pre-existing. Threat model
  assumes trusted config file authors. We add scheme validation
  (https preferred, http:// warned for non-localhost) but no host
  filtering.
• Patchwork permissions scope unresolved -- upstream limitation.
  Email mode exists as the mitigation for subsystems that can't
  accept the broad token scope.
• No integration test for PatchworkWorker -- requires mock HTTP
  server. Validated manually against production patchwork.kernel.org
  MPTCP instance (6-patch series, all checks posted successfully).
  Unit tests cover all components individually.
• Config file read per delivery attempt -- the PatchworkWorker
  reads email_policy.toml from disk each time it resolves a token.
  Same pattern the reviewer already uses. Negligible I/O at the
  expected volume.

Pre-existing issues fixed opportunistically

1. EmailOutboxRow.patch_id type mismatch (i64 vs nullable schema)
2. Message-ID not URL-encoded in patchwork API queries
3. Multiple patches for same msgid silently used first without warning

Testing

• 33 new unit tests (check state mapping, description format,
  pre-existing handling, severity filtering, config deserialization,
  URL normalization, token override, email composition)
• Validated against production patchwork.kernel.org MPTCP instance:
  6 patches posted via PatchworkWorker code path (4 success,
  2 warning), all verified via authenticated API GET
• make check-pr passes (301 unit + 17 integration, all green)
• No new dependencies added

[rgushchin]

Overall looks pretty good to me, thanks! It's in the draft state, anything big before it can be merged? Has it been tested with the real patchwork instance?

[derekbarbosa]

@rgushchin pushed it up before leaving for vacation so haven't had real time to test it

when I get back I'll plan to grab those credentials from you and run some trials with --no-ai.

Hopefully Mauro's PR to change the scoping of PW tokens will have been merged as well.

getpatchwork/patchwork#653

[derekbarbosa]

v2:

The PatchworkWorker now resolves tokens from the email policy config (and
SASHIKO_PATCHWORK_TOKEN env var) at delivery time by matching the outbox row's
api_url against subsystem policies.

adds a min_severity field to the patchwork policy. Findings below the configured
threshold are excluded from the patchwork check count. The
status/description computation was moved inside the per-policy loop so each
subsystem can apply its own threshold independently.

[derekbarbosa]

@matttbe requesting your review.

tested on following series:
https://patchwork.kernel.org/project/mptcp/list/?series=1101575&archive=both&state=*

test methodology:

• Fresh sashiko database with empty patchwork_outbox table
• email_policy.toml configured with an MPTCP subsystem pointing at
  https://patchwork.kernel.org/api/1.3
• SASHIKO_PATCHWORK_TOKEN environment variable set
• Sashiko started with --no-ai (no AI reviews) and without --track
  to isolate PatchworkWorker

each patch was then polled at the prod sashiko instance.

1. INSERT a single row into patchwork_outbox with the patch's
   message-ID, the correct check state and description from the
   actual sashiko review
2. Verify the row exists with correct data
3. Start sashiko and observe the PatchworkWorker pick up the row
4. Verify the row status changed to Sent in the database
5. Verify the check appeared on patchwork.kernel.org via
   authenticated API GET
6. Stop sashiko, clear the database, proceed to next patch

sample email policy:

[subsystems.mptcp]
lists = ["mptcp@lists.linux.dev"]
mute_all = true

[subsystems.mptcp.patchwork]
enabled = true
api_url = "https://patchwork.kernel.org/api/1.3"

@rgushchin could you lend me your eyes one more time for a review on this V2? thanks!

logs from manual cURL confirmation:

Patch 14597067:
  id=10391949  user=sashiko  state=success  date=2026-06-18T02:48:43.359090
  description=Sashiko AI review found no regressions
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=1
Patch 14597067:
  id=10392308  user=sashiko  state=success  date=2026-06-18T03:06:49.829832
  description=Sashiko AI review found no regressions
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=1
Patch 14597068:
  id=10392332  user=sashiko  state=warning  date=2026-06-18T03:10:15.769390
  description=Sashiko AI review found 2 potential issue(s)
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=2
Patch 14597069:
  id=10392349  user=sashiko  state=success  date=2026-06-18T03:18:27.047327
  description=Sashiko AI review found no regressions
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=3
Patch 14597070:
  id=10392350  user=sashiko  state=success  date=2026-06-18T03:19:47.416709
  description=Sashiko AI review found no regressions
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=4
Patch 14597072:
  id=10392351  user=sashiko  state=success  date=2026-06-18T03:21:02.864771
  description=Sashiko AI review found no regressions
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=5
Patch 14597071:
  id=10392363  user=sashiko  state=warning  date=2026-06-18T03:22:24.332710
  description=Sashiko AI review found 2 potential issue(s)
  target_url=https://sashiko.dev/#/patchset/cover.1779876523.git.pabeni@redhat.com?part=6

Please send me any other candidates you think are worthwhile to test :)

[rgushchin]

Some comments from AI, look reasonable to me (especially the first one):

1. Blocking Retry Loop in PatchworkWorker (High Risk): The sleep in the retry error path blocks the entire worker loop.
   If one Patchwork instance is down, it blocks pending updates for all other subsystems. Recommend using a database-backed
   next_retry_at timestamp for non-blocking retries.
2. Hardcoded max_retries : PatchworkWorker hardcodes max_retries to 3 instead of using the configured settings.
   review.max_retries from Settings.toml .
3. Insecure Schemes: The URL normalization allows http:// . Consider restricting to https:// (or warning) for non-
   localhost destinations.

[matttbe]

Thank you for working on that, that looks good!

Regarding the descriptions that are sent, how is it going to cover the "pre-existing issues"?

Personally, I think it would be good to:

• set something like "Critical: 1 · High: 0 (2) · Medium: 0 · Low: 0" in the description (or something shorter, or drop the ones at 0)
• set the state depending only on new issues: critical and high as fail, medium and low only as warning, and success is no new issues (but keep the description with existing issues if any).
• (but it feels like maybe other maintainers will have different opinions: maybe some categories will be ignored, maybe pre-existing issues are as important as new ones, etc. but this could be changed later.)

But this suggestion can also be changed later, I don't want to block this PR.

[derekbarbosa]

v2->v3:
incorporated @rgushchin review comments
incorporated @matttbe review comments

overview of changes

• Per-severity description with pre-existing awareness: v2
  used a flat count ("found 2 potential issue(s)"). v3 shows a
  per-severity breakdown with pre-existing counts in parentheses,
  dropping zero-count severities. Example:
  Critical: 1 · High: 2 (1 pre-existing). Pre-existing findings
  are always shown in the description so maintainers have full
  context, but they never affect the check state.

• Configurable check state mapping via fail_severity: v2
  only used success and warning. v3 adds the fail state,
  controlled by fail_severity (default: "High"). New findings
  at or above the threshold produce fail; below it produce
  warning. Pre-existing findings never affect state. This
  addresses the feedback that Critical/High issues should be
  flagged more strongly than Medium/Low.

• PatchworkCheckResult builder pattern: v2 computed the check
  state and description inline in reviewer.rs using
  count_filtered_findings() on PatchworkPolicy. v3 extracts
  this into a PatchworkCheckResult::from_policy() builder in
  patchwork.rs, keeping PatchworkPolicy as a pure config struct
  and separating the computation logic into its own type.

• Non-blocking retry via next_retry_at: v2 blocked the worker
  loop with sleep() during retry backoff, preventing other
  entries from being processed. v3 replaces the sleep with a
  next_retry_at timestamp column in patchwork_outbox. Failed
  entries are immediately returned to Pending with a future
  timestamp, and lock_pending_patchwork() skips rows where
  next_retry_at > now. The worker loop continues immediately.

• max_retries from settings: v2 hardcoded max_retries: 3
  in PatchworkWorker. v3 reads settings.review.max_retries
  (default: 3) and passes it to the worker constructor.

• Non-localhost http:// warning: v2 accepted http:// URLs
  without comment. v3 warns when api_url uses http:// for
  non-localhost hosts, since the API token would be sent in
  plaintext. Localhost (127.0.0.1, localhost, [::1]) is accepted
  silently for local development.

• Edge case behaviors documented: Missing or null
  preexisting flag treated as new finding. min_severity
  filtering applies to both new and pre-existing counts. When
  only pre-existing findings remain, state is success with
  the pre-existing breakdown in the description.

[derekbarbosa]

rebased on latest main