refactor(#2416): remove the non-pr config-change flow (request_apply_commit / applycommit)

This commit is contained in:
damocles 2026-07-15 20:45:29 +02:00 committed by mara
commit c2bd7db998
34 changed files with 293 additions and 1635 deletions

View file

@ -1,13 +1,8 @@
//! Approval endpoints + diff machinery for the dashboard.
//! Approval endpoints for the dashboard.
//!
//! Approve/deny actions, the orphan-approval GC sweep used by the
//! `/api/state` builder, and the unified-diff endpoints (on-demand
//! `/api/approval-diff/{id}` against a chosen base, plus the `pub(crate)`
//! `approval_diff` the manager-socket handler pre-computes at submit time).
//! Approve/deny actions plus the orphan-approval GC sweep used by the
//! `/api/state` builder.
use std::path::Path;
use anyhow::{Context, Result};
use axum::{
extract::{Form, Path as AxumPath, State},
http::StatusCode,
@ -16,12 +11,9 @@ use axum::{
use hive_sh4re::Approval;
use serde::Deserialize;
use problem_details::ProblemDetails;
use super::{AppState, error_problem, error_response};
use super::{AppState, error_response};
use crate::actions;
use crate::coordinator::Coordinator;
use crate::lifecycle;
pub(super) async fn post_approve(
State(state): State<AppState>,
@ -53,7 +45,7 @@ pub(super) async fn post_deny(
.as_deref()
.map(str::trim)
.filter(|s| !s.is_empty());
match actions::deny(&state.coord, id, note).await {
match actions::deny(&state.coord, id, note) {
Ok(()) => (StatusCode::OK, "ok").into_response(),
Err(e) => error_response(&format!("deny {id} failed: {e:#}")),
}
@ -87,7 +79,7 @@ pub(super) fn gc_orphans(coord: &Coordinator, approvals: Vec<Approval>) -> Vec<A
coord.emit_approval_resolved(crate::coordinator::ApprovalResolved {
id: a.id,
agent: &a.agent,
approval_kind: "apply_commit",
approval_kind: a.kind.as_str(),
sha_short,
status: "failed",
note: Some(note.to_owned()),
@ -98,142 +90,3 @@ pub(super) fn gc_orphans(coord: &Coordinator, approvals: Vec<Approval>) -> Vec<A
})
.collect()
}
/// Multi-file unified diff between the currently-deployed tree and
/// the proposal for this approval. Runs against the applied repo
/// since the canonical proposal commit lives there (manager-side
/// amendments don't move it). Empty output means proposal == main —
/// a no-op approval.
///
/// `pub(crate)` so the manager-socket handler can pre-compute the
/// diff once at submission time and embed it in the `ApprovalAdded`
/// dashboard event (instead of forcing the dashboard to wait a
/// `/api/state` cycle to see the diff for newly-queued approvals).
pub(crate) async fn approval_diff(agent: &str, approval_id: i64) -> String {
let applied = crate::paths::applied_dir(agent);
if !applied.join(".git").exists() {
return format!("(no applied git repo at {})", applied.display());
}
let proposal_ref = format!("refs/tags/proposal/{approval_id}");
match git_diff_refs(&applied, "refs/heads/main", &proposal_ref).await {
Ok(s) if s.is_empty() => "(proposal matches currently-deployed tree)".to_owned(),
Ok(s) => s,
Err(e) => format!("(error: {e:#})"),
}
}
async fn git_diff_refs(applied_dir: &Path, base_ref: &str, target_ref: &str) -> Result<String> {
let out = lifecycle::git_command()
.current_dir(applied_dir)
.args(["diff", &format!("{base_ref}..{target_ref}")])
.output()
.await
.with_context(|| format!("spawn `git diff` in {}", applied_dir.display()))?;
if !out.status.success() {
anyhow::bail!(
"git diff {base_ref}..{target_ref} failed: {}",
String::from_utf8_lossy(&out.stderr).trim()
);
}
Ok(String::from_utf8_lossy(&out.stdout).into_owned())
}
/// Numeric ids of `<prefix>/<n>` tags in the applied repo (e.g.
/// `proposal/3` → `3`). Unparseable suffixes are skipped. Used to
/// resolve the `approved` / `previous` diff bases for an approval.
async fn tag_ids(applied_dir: &Path, prefix: &str) -> Vec<i64> {
let Ok(out) = lifecycle::git_command()
.current_dir(applied_dir)
.args(["tag", "-l", &format!("{prefix}/*")])
.output()
.await
else {
return Vec::new();
};
if !out.status.success() {
return Vec::new();
}
let strip = format!("{prefix}/");
String::from_utf8_lossy(&out.stdout)
.lines()
.filter_map(|l| l.trim().strip_prefix(&strip))
.filter_map(|s| s.parse::<i64>().ok())
.collect()
}
#[derive(Deserialize)]
pub(super) struct DiffBaseQuery {
/// `applied` (running tree — default), `approved` (most recent
/// earlier approved proposal), or `previous` (the prior queued
/// proposal for this agent).
base: Option<String>,
}
/// On-demand unified diff for one `ApplyCommit` approval against a
/// chosen base. `applied` = `applied/main` (what's running);
/// `approved` = the most recent earlier `approved/<n>` tag (the last
/// proposal the operator OK'd, even if its build then failed);
/// `previous` = the prior queued `proposal/<n>` (the incremental
/// delta when the manager chains proposals). Returns the raw diff
/// text — the dashboard classifies lines client-side.
pub(super) async fn get_approval_diff(
State(state): State<AppState>,
AxumPath(id): AxumPath<i64>,
axum::extract::Query(q): axum::extract::Query<DiffBaseQuery>,
) -> Result<Response, ProblemDetails> {
let base = q.base.as_deref().unwrap_or("applied");
let approval = match state.coord.approvals.get(id) {
Ok(Some(a)) => a,
Ok(None) => return Err(error_problem(&format!("approval {id} not found"))),
Err(e) => return Err(error_problem(&format!("approval {id}: {e:#}"))),
};
if !matches!(approval.kind, hive_sh4re::ApprovalKind::ApplyCommit) {
return Err(error_problem("spawn approvals carry no commit to diff"));
}
let applied = crate::paths::applied_dir(&approval.agent);
if !applied.join(".git").exists() {
return Ok(plain_text(format!(
"(no applied git repo at {})",
applied.display()
)));
}
let target = format!("refs/tags/proposal/{id}");
let base_ref = match base {
"applied" => Some("refs/heads/main".to_owned()),
"approved" => {
let ids = tag_ids(&applied, "approved").await;
ids.into_iter()
.filter(|&n| n != id)
.max()
.map(|n| format!("refs/tags/approved/{n}"))
}
"previous" => {
let ids = tag_ids(&applied, "proposal").await;
ids.into_iter()
.filter(|&n| n < id)
.max()
.map(|n| format!("refs/tags/proposal/{n}"))
}
other => {
return Err(ProblemDetails::from_status_code(StatusCode::BAD_REQUEST)
.with_detail(format!("unknown diff base {other:?}")));
}
};
let Some(base_ref) = base_ref else {
return Ok(plain_text(match base {
"approved" => "(no earlier approved proposal to diff against)".to_owned(),
_ => "(no previous proposal to diff against)".to_owned(),
}));
};
match git_diff_refs(&applied, &base_ref, &target).await {
Ok(s) if s.is_empty() => Ok(plain_text(
"(identical — no changes vs this base)".to_owned(),
)),
Ok(s) => Ok(plain_text(s)),
Err(e) => Err(error_problem(&format!("git diff: {e:#}"))),
}
}
fn plain_text(body: String) -> Response {
(StatusCode::OK, body).into_response()
}

View file

@ -202,7 +202,7 @@ pub(super) async fn post_request_spawn(
tracing::info!(%id, %name, "operator: spawn approval queued via dashboard");
// Phase 5b: notify the dashboard event channel so live
// subscribers can append the row without a snapshot
// refetch. Spawn approvals carry no diff/sha.
// refetch. Spawn approvals carry no sha.
state
.coord
.emit_approval_added(crate::coordinator::ApprovalAdded {
@ -210,7 +210,6 @@ pub(super) async fn post_request_spawn(
agent: &name,
approval_kind: "spawn",
sha_short: None,
diff: None,
description: None,
pr_number: None,
});

View file

@ -35,11 +35,6 @@ mod tombstones;
mod topology;
mod webhook;
// Pre-computed at approval-submit time by the manager-socket handler
// (`socket_server.rs`) and embedded in the `ApprovalAdded` event, so
// re-exported at the module root to preserve the `crate::dashboard::approval_diff`
// path across the submodule split.
pub(crate) use approvals::approval_diff;
// Run after lock bumps by the job queue (`job_queue/exec.rs`); the view
// type feeds `DashboardEvent::MetaInputsChanged` (`dashboard_events.rs`).
// Re-exported to preserve the `crate::dashboard::*` paths across the split.
@ -83,7 +78,6 @@ pub async fn serve(
.route("/api/state", get(state_snapshot::api_state))
.route("/api/journal/{name}", get(journal::get_journal))
.route("/api/journal-host", get(journal::get_journal_host))
.route("/api/approval-diff/{id}", get(approvals::get_approval_diff))
.route("/api/state-file", get(state_files::get_state_file))
.route(
"/api/matrix-accounts",

View file

@ -23,7 +23,7 @@ use crate::container_view::ContainerView;
use super::meta_inputs::{MetaInputView, read_meta_inputs};
use super::tombstones::{TombstoneView, build_tombstone_views};
use super::{AppState, approval_diff, approvals, error_response, scan_validated_paths};
use super::{AppState, approvals, error_response, scan_validated_paths};
#[allow(clippy::struct_excessive_bools)]
#[derive(Serialize)]
@ -233,31 +233,16 @@ struct ApprovalView {
id: i64,
agent: String,
kind: &'static str,
/// First 12 chars of the `commit_ref`, for `ApplyCommit` only.
/// Display-only (the short chip on the card).
/// First 12 chars of the reviewed PR head sha, for `MergeConfigPr`
/// only. Display-only (the short chip on the card).
sha_short: Option<String>,
/// Full commit sha, for `ApplyCommit` only. The frontend builds the
/// "commit on forge" link from this rather than `sha_short`: forgejo
/// 404s an abbreviated sha for the proposal commit (it lives on a
/// `proposal/<id>` tag ref, which forgejo won't disambiguate a short
/// hash against), but resolves the full 40-char sha by direct lookup.
#[serde(skip_serializing_if = "Option::is_none")]
sha_full: Option<String>,
/// Raw unified diff text, for `ApplyCommit` only. The client splits
/// on `\n` and per-line classifies (`+` / `-` / `@@` / `--- ` / `+++ `
/// → diff-add / diff-del / diff-hunk / diff-file). Shipping raw
/// instead of pre-rendered HTML saves bytes on the wire (no
/// per-line `<span>` markup) and removes the only HTML-escape
/// surface from the snapshot.
diff: Option<String>,
/// Manager-supplied description shown on the approval card.
#[serde(skip_serializing_if = "Option::is_none")]
description: Option<String>,
/// Forge PR number, for `MergeConfigPr` only. Lets the frontend
/// build a "review PR on forge" link
/// (`{forgeBase}/agent-configs/{agent}/pulls/{pr_number}`) the same
/// way it builds the `apply_commit` "commit on forge" link from the
/// sha. `None` for every other kind.
/// (`{forgeBase}/agent-configs/{agent}/pulls/{pr_number}`). `None`
/// for every other kind.
#[serde(skip_serializing_if = "Option::is_none")]
pr_number: Option<u64>,
/// Raw `commit_ref` payload for `UpdateMetaInputs` (JSON-encoded
@ -352,7 +337,7 @@ pub(super) async fn api_state(
log_default("approvals.pending", state.coord.approvals.pending()),
);
let transients = build_transient_views(&containers, &transient_snapshot);
let approvals = build_approval_views(pending_approvals).await;
let approvals = build_approval_views(pending_approvals);
let approval_history = log_default(
"approvals.recent_resolved",
state.coord.approvals.recent_resolved(30),
@ -544,8 +529,8 @@ fn transient_label(k: crate::coordinator::TransientKind) -> &'static str {
}
}
/// Render each pending approval into its dashboard view (short sha +
/// unified diff for `ApplyCommit`, just the name for `Spawn`).
/// Render each pending approval into its dashboard view (short sha for
/// `MergeConfigPr`, just the name for `Spawn`).
/// Project a resolved sqlite row into the lean shape the dashboard
/// history tab consumes — no `diff_html` (rendering 30 of them
/// per /api/state poll would mean 30 git diffs per refresh).
@ -576,37 +561,15 @@ fn history_view(a: Approval) -> ApprovalHistoryView {
}
}
async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
let mut out = Vec::with_capacity(approvals.len());
for a in approvals {
out.push(match a.kind {
hive_sh4re::ApprovalKind::ApplyCommit => {
// Prefer the canonical fetched sha from applied;
// commit_ref is only the manager's claim and may be
// amended out from under us.
let displayed = a.fetched_sha.as_deref().unwrap_or(&a.commit_ref);
let sha = displayed[..displayed.len().min(12)].to_owned();
let diff = approval_diff(&a.agent, a.id).await;
ApprovalView {
id: a.id,
agent: a.agent.clone(),
kind: "apply_commit",
sha_short: Some(sha),
sha_full: Some(displayed.to_owned()),
diff: Some(diff),
description: a.description,
pr_number: None,
commit_ref: None,
requested_at: a.requested_at,
}
}
hive_sh4re::ApprovalKind::Spawn => ApprovalView {
id: a.id,
agent: a.agent,
kind: "spawn",
sha_short: None,
sha_full: None,
diff: None,
description: a.description,
pr_number: None,
commit_ref: None,
@ -617,8 +580,6 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
agent: a.agent,
kind: "init_config",
sha_short: None,
sha_full: None,
diff: None,
description: a.description,
pr_number: None,
commit_ref: None,
@ -629,8 +590,6 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
agent: a.agent,
kind: "update_meta_inputs",
sha_short: None,
sha_full: None,
diff: None,
description: a.description,
pr_number: None,
commit_ref: Some(a.commit_ref),
@ -641,8 +600,6 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
agent: a.agent,
kind: "schedule_prompt",
sha_short: None,
sha_full: None,
diff: None,
description: a.description,
pr_number: None,
commit_ref: Some(a.commit_ref),
@ -650,8 +607,8 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
},
hive_sh4re::ApprovalKind::MergeConfigPr => {
// commit_ref = PR number; fetched_sha = the reviewed PR
// head. Show the head sha; the forge PR diff surface is
// a later phase of the PR-based config flow — None for now.
// head. Show the head sha; the config diff surface lives
// on the forge PR itself.
let sha = a
.fetched_sha
.as_deref()
@ -664,8 +621,6 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
agent: a.agent,
kind: "merge_config_pr",
sha_short: sha,
sha_full: None,
diff: None,
description: a.description,
pr_number,
commit_ref: None,