feat: Branch Gardener scheduled branch-hygiene skill (#14) - #65
Conversation
…labels (#14) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
…ng (#14) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
…rface draft PRs (#14) Replace the GardenRun stub with a real execution path: scan branches against the resolved per-repo policy, classify and plan, prune dead and merged branches (re-checking the SHA before every delete), surface PR-less human branches as labeled draft PRs, and post the run report on the report issue. Bot-only PR-less branches are skipped, and the whole runner stays behind the write-permission command gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
…ebase Main added backend/container/allow_host_backend to WorkerConfig (#59) while this branch added the gardener section to Config; fill both sides in the test literals that construct the structs directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces the “Branch Gardener” feature to the coven-github adapter: a new gardener crate for branch classification/planning/reporting, plus webhook/worker plumbing to run gardener scans and optionally prune dead/merged branches under policy.
Changes:
- Adds a new
coven-github-gardenercrate implementing schedule parsing, branch classification/planning, and markdown reporting. - Extends config with a
[gardener]policy (enabled/autonomy/schedule/excludes/draft PR label + per-repo overrides) and doctor validation. - Adds a new
GardenRuntask kind routed from thegardencommand and executed adapter-side by the worker.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/worker/src/lib.rs | Routes TaskKind::GardenRun to an adapter-only execution path and adds an AdapterCompleted preflight outcome. |
| crates/worker/src/gardener_exec.rs | Implements the garden run scan/plan/execute/report flow, including delete-with-SHA-recheck and draft-PR surfacing. |
| crates/worker/src/brief.rs | Prevents briefing GardenRun tasks into coven-code by panicking if attempted. |
| crates/worker/Cargo.toml | Adds dependency on the new gardener crate. |
| crates/webhook/src/routes.rs | Routes @bot garden to a GardenRun task with the invoking surface as the report issue. |
| crates/webhook/src/commands.rs | Adds garden to the command grammar and help list. |
| crates/github/src/tasks.rs | Makes task “surface” optional to support scheduled garden runs with no single issue/PR surface. |
| crates/github/src/repo.rs | Adds branch listing, compare-ahead/behind, and ref deletion GitHub API calls. |
| crates/github/src/pr.rs | Adds head-PR lookup and label application helpers used by the gardener executor. |
| crates/github/src/lib.rs | Adds ref-component encoding helper used by the new GitHub API request builders. |
| crates/gardener/src/schedule.rs | Implements restricted cron parsing and stable slot IDs for scheduled runs. |
| crates/gardener/src/scan.rs | Implements branch classification, exclude matching, and action planning. |
| crates/gardener/src/report.rs | Implements markdown run reporting and summary lines. |
| crates/gardener/src/lib.rs | Exposes the gardener crate’s modules and public API surface. |
| crates/gardener/Cargo.toml | Introduces the new coven-github-gardener crate package manifest. |
| crates/config/src/lib.rs | Adds [gardener] config types, effective-policy resolution, and doctor validation. |
| crates/config/Cargo.toml | Adds dependency on the gardener crate for policy parsing/validation. |
| Cargo.toml | Adds crates/gardener to the workspace members. |
| Cargo.lock | Records new workspace crate dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn compare_request(owner: &str, name: &str, base: &str, head: &str) -> GitHubRequest { | ||
| let base = crate::encode_ref_component(base); | ||
| let head = crate::encode_ref_component(head); | ||
| GitHubRequest { | ||
| method: "GET", | ||
| path: format!("/repos/{owner}/{name}/compare/{base}...{head}"), | ||
| body: serde_json::Value::Null, | ||
| } |
| let compare = repo::compare_ahead_behind_with_base_url( | ||
| api_base_url, | ||
| orchestration, | ||
| &task.repo_owner, | ||
| &task.repo_name, |
| let pulls = pr::list_pulls_by_head_with_base_url( | ||
| api_base_url, | ||
| orchestration, | ||
| &task.repo_owner, | ||
| &task.repo_name, |
| warn!( | ||
| task_id = %task.id, | ||
| branch = %action.branch, | ||
| "skipping branch prune after delete failed: {error:#}" | ||
| ); |
| if !plan.surface.is_empty() { | ||
| let publication = minter.mint(TokenRole::Publication).await?; | ||
| let planned_surface = std::mem::take(&mut plan.surface); | ||
| for mut action in planned_surface { | ||
| info!( |
BunsDev
left a comment
BunsDev
left a comment
There was a problem hiding this comment.
Thorough pass over the Branch Gardener — classifier, planner, executor, GitHub helpers, config/validation, and the test suite. Overall this is strong, safety-conscious work: the pre-delete SHA re-check (TOCTOU guard), the deny-by-default enabled=false, protected+default-branch exclusion, the conservative is_proven_bot_only (fails toward surfacing), and the token scoping are all right. On scoping in particular — prune mints AgentGit (contents: write only, exactly what DELETE /git/refs needs) and surface mints Publication (pull_requests: write); that's correct least-privilege. The tests genuinely assert the safety properties rather than just exercising them (propose asserts zero DELETEs, prune-dead asserts exactly [dead, merged], moved-SHA asserts no delete + counter, drive-by asserts decline before any work).
One thing worth a conscious maintainer decision plus a few smaller items, inline. The headline: the Trust-posture line "propose (dry-run) … proven no-write by test" is not accurate — propose is proven no-delete, but it still opens (and labels) a draft PR for every PR-less human branch. Your own test at lines 3119-3141 asserts exactly that write happens under propose. Worth either gating surfacing behind a would_surface bucket so propose is truly no-write, or correcting the description + config docs so an operator enabling the default tier isn't surprised by a burst of draft PRs. Details inline. None of these are blocking correctness bugs; the delete path is safe.
| if is_proven_bot_only(branch) { | ||
| plan.skipped.push(skip(branch, SkipCode::BotOnlyPrless)); | ||
| } else { | ||
| plan.surface.push(SurfaceAction { |
There was a problem hiding this comment.
Surfacing is not gated by autonomy — propose still writes. Merged/Dead route through the autonomy gate just above (137-142: would_prune vs prune), but Prless goes straight to plan.surface with no such gate, and execute_garden_plan unconditionally opens a draft PR + applies a label for everything in surface (gardener_exec.rs:300-365). So under the default propose tier, enabling the gardener on a repo with N PR-less human branches opens N draft PRs on the first run.
This is intended-and-tested (propose_garden_run_...surfaces_prless asserts the POST /pulls at lines 3123-3130), so it's not a code bug — but it contradicts the PR description's "propose (dry-run) … proven no-write by test." It's proven no-delete; it does write draft PRs.
Two clean options: (a) add a would_surface bucket symmetric to would_prune so propose is genuinely no-write and surfacing only happens at prune-dead (or a new tier); or (b) keep the behavior but fix the Trust-posture wording and document in the [gardener] config that propose opens draft PRs and only deletion is gated. Either is fine — but the current "dry-run / no-write" framing will mislead an operator turning this on.
| return BranchClass::Excluded; | ||
| } | ||
|
|
||
| if facts.open_pr.is_some() { |
There was a problem hiding this comment.
Idempotency: a human-closed (rejected) surface PR gets re-opened every run. open_pr only counts state == "open" pulls (executor line 163-166), so a branch whose gardener draft PR a maintainer closed without merging classifies as Prless again next run (open_pr=None, merged_pr=None, ahead>0) and is surfaced anew. Steady state within a run is fine — an open draft PR makes the branch Active — but across scheduled runs a "no thanks" close isn't remembered, so the gardener re-nags with a fresh draft PR.
Consider skipping branches that already have a closed, unmerged PR from the gardener (e.g. detect the draft-PR label, or treat any recent closed head PR as a decline), or at minimum note it as a known limitation. Not blocking, but it'll be a papercut on active repos.
| action = "prune", | ||
| "branch gardener pruning branch" | ||
| ); | ||
| let current_sha = match repo::get_branch_sha_with_base_url( |
There was a problem hiding this comment.
Heads-up: this pre-delete SHA re-check uses a helper that doesn't percent-encode the branch. get_branch_sha_with_base_url → get_branch_request (repo.rs) interpolates {branch} raw, unlike compare_request and delete_ref_request, which both run it through encode_ref_component. That helper is pre-existing (from the #10 ref-resolution work), so this isn't something this PR introduced — but this PR is the first to put it on a destructive path, so it's worth noting here.
For a branch name with a URL-significant char (#, ?, space), the lookup resolves the wrong path or 404s. The direction is safe — a failed/mismatched SHA lookup is caught and counted as prune_skipped_moved (256-264), so nothing wrong is deleted — but such a branch can then never be pruned even when legitimately dead. Consider hardening get_branch_request to encode the ref for parity with its siblings (slashes are preserved, so ordinary feat/x names are unaffected).
| facts | ||
| .iter() | ||
| .find(|fact| fact.name == action.branch) | ||
| .expect("planned prune action has source facts"), |
There was a problem hiding this comment.
.expect() inside the mutation loop. If plan.prune ever references a branch absent from facts, this panics the worker mid-run (after possibly having already deleted earlier branches). It's currently unreachable — plan.prune is built from facts in the same pass — so this is latent, purely defensive. But an expect in a delete loop is a sharp edge: prefer find(...).map(...) and, on None, log + continue (skip the prune) rather than unwind. This block is also only reachable to compute a log field for classification, which makes a hard panic there especially not worth it.
Wire configured gardener schedules into the server, support /coven garden, and update shipped docs/status. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
BunsDev
changed the title
Implements the Branch Gardener branch-hygiene skill from issue #14 (
docs/branch-gardener.md, ROADMAP M2) — complete, including the scheduled trigger.What's here
coven-github-gardenercrate — branch classifier (active / merged / dead / PR-less / excluded), policy planner, run report, and cron-style schedule parsing.[gardener]section with per-repo overrides:enabled,autonomy(propose|prune-dead),schedule,excludeglobs,draft_pr_label; validated by doctor; documented inconfig/example.tomlanddocs/self-hosting.md.gardencommand (/coven garden) producing an adapter-onlyGardenRuntask kind, behind the write-permission command gate (Add marker-backed comments and maintainer command protocol #13).prune-deadautonomy and re-checks the branch SHA immediately before every delete (moved branches are skipped and counted), surfaces PR-less human branches as labeled draft PRs, skips bot-only branches, and posts a structured run report on the report issue.GardenRuntasks per repo off the durable queue (grooming item 5).Trust posture
propose(dry-run) is the default autonomy — proven no-write by test.Verification
cargo check --all-targets,cargo clippy --all-targets -- -D warnings, andcargo test --allgreen locally and in CI on the head SHA (mock-server coverage: propose makes no deletes, prune-dead deletes only dead+merged, moved-SHA guard, draft-PR surfacing + labels, bot-only skip, below-write decline, disabled-repo no-scan, scheduler enqueue).Closes #14.