Deploying Slice 5 and approving one roster in production broke it twice, in ways
528 green tests had nothing to say about.
**1. `jsonb_set` refuses a scalar.** A mission created through the API without a
`config` stores jsonb `null` — a scalar — and `jsonb_set` fails on it with
"cannot set path in scalar". The guard was `coalesce(config, '{}')`, which
protects against SQL NULL; this is a perfectly good JSON null of the wrong shape,
and coalesce passes it straight through. Every test wrote `'{}'::jsonb` because
that is what a test author types. Production types nothing at all.
**2. The approval was not atomic, and failing halfway is permanent.** The claim
and the mission write were two statements, claim first, so when the write failed
the proposal stood `approved` with nothing applied — and the partial unique index
then makes that state unrecoverable: no other proposal for that mission can ever
be approved. The mission ran solo with `team_engine` still NULL while its
proposal said otherwise.
`approve_and_apply` is now one transaction: claim, write, commit or roll back.
The type guard is `CASE WHEN jsonb_typeof(config) = 'object' THEN config ELSE
'{}'::jsonb END`, which answers the question that was actually being asked.
Both regressions are tested in the shape production had, and both NEGATIVE
CONTROLS were run rather than assumed:
- restore `coalesce` → `a_roster_applies_to_a_mission_whose_config_is_json_null`
FAILS with Postgres's own "cannot set path in scalar", the exact production
error.
- commit instead of roll back on a failed apply →
`a_failed_apply_leaves_the_proposal_undecided` FAILS with the proposal stuck
`approved`.
Worth stating plainly: the API returned 500 for that approval, so this was not
silent to the caller — but the row it left behind claimed the mission had a
roster it never received, and the mission then ran and delivered, which is the
shape that gets believed.
530 tests pass, clippy clean.
Co-Authored-By: Claude Opus 5 <[email protected]>
241 lines
9.0 KiB
Rust
241 lines
9.0 KiB
Rust
//! A mission may have many proposals and at most one approved roster.
|
|
//!
|
|
//! Both properties are enforced in SQL rather than in the handler, and both
|
|
//! matter for the same reason: the composed executor reads ONE field for what
|
|
//! shape a mission is, so a second approval would silently win by being written
|
|
//! last.
|
|
|
|
use cm_db::repo::mission_team_proposals as proposals;
|
|
use cm_domain::WorkspaceId;
|
|
use serde_json::json;
|
|
use uuid::Uuid;
|
|
|
|
async fn workspace(pool: &sqlx::PgPool) -> WorkspaceId {
|
|
let ws = cm_domain::Workspace {
|
|
id: WorkspaceId::new(),
|
|
name: "Roster".into(),
|
|
plan: "team".into(),
|
|
};
|
|
cm_db::repo::workspaces::insert(pool, &ws).await.expect("workspace");
|
|
ws.id
|
|
}
|
|
|
|
/// A mission row to hang proposals off — `mission_id` is a real FK.
|
|
async fn mission(pool: &sqlx::PgPool, ws: WorkspaceId) -> Uuid {
|
|
let id = Uuid::now_v7();
|
|
sqlx::query(
|
|
"INSERT INTO missions (id, workspace_id, title, template_kind, status, schedule, config)
|
|
VALUES ($1, $2, 'roster test', 'research_and_code', 'draft', '{}'::jsonb, '{}'::jsonb)",
|
|
)
|
|
.bind(id)
|
|
.bind(ws.as_uuid())
|
|
.execute(pool)
|
|
.await
|
|
.expect("insert mission");
|
|
id
|
|
}
|
|
|
|
fn roster() -> serde_json::Value {
|
|
json!({
|
|
"topology_kind": "pipeline",
|
|
"members": [
|
|
{"role": "implementer"},
|
|
{"role": "verifier", "backend": "kimi"}
|
|
]
|
|
})
|
|
}
|
|
|
|
/// The whole point of persisting: a proposal is a record, not a click. It
|
|
/// arrives `proposed`, applied to nothing.
|
|
#[tokio::test]
|
|
async fn a_proposal_arrives_undecided_and_is_listed() {
|
|
let pool = cm_testkit::test_pool().await;
|
|
let ws = workspace(&pool).await;
|
|
let m = mission(&pool, ws).await;
|
|
|
|
let id = Uuid::now_v7();
|
|
proposals::insert(&pool, id, m, ws.as_uuid().to_owned(), &roster(), "claude-opus-4-8")
|
|
.await
|
|
.expect("insert");
|
|
|
|
let rows = proposals::list(&pool, m, ws.as_uuid().to_owned()).await.expect("list");
|
|
assert_eq!(rows.len(), 1);
|
|
assert_eq!(rows[0].status, "proposed");
|
|
assert_eq!(rows[0].author_model, "claude-opus-4-8");
|
|
assert!(rows[0].decided_at.is_none());
|
|
assert_eq!(rows[0].roster["members"][1]["backend"], "kimi");
|
|
}
|
|
|
|
/// Deciding twice must not decide twice. A double-clicked approve, or a retried
|
|
/// request, would otherwise re-apply a roster to a mission that has moved on.
|
|
#[tokio::test]
|
|
async fn only_the_first_decision_counts() {
|
|
let pool = cm_testkit::test_pool().await;
|
|
let ws = workspace(&pool).await;
|
|
let m = mission(&pool, ws).await;
|
|
let id = Uuid::now_v7();
|
|
proposals::insert(&pool, id, m, ws.as_uuid().to_owned(), &roster(), "claude-opus-4-8")
|
|
.await
|
|
.expect("insert");
|
|
|
|
let first = proposals::decide(&pool, id, ws.as_uuid().to_owned(), "approved", None, None)
|
|
.await
|
|
.expect("decide");
|
|
assert!(first, "the first approval must claim the proposal");
|
|
|
|
let second = proposals::decide(&pool, id, ws.as_uuid().to_owned(), "rejected", None, None)
|
|
.await
|
|
.expect("decide");
|
|
assert!(!second, "a decided proposal must not be re-decided");
|
|
|
|
let rows = proposals::list(&pool, m, ws.as_uuid().to_owned()).await.expect("list");
|
|
assert_eq!(rows[0].status, "approved", "and the first decision stands");
|
|
assert!(rows[0].decided_at.is_some());
|
|
}
|
|
|
|
/// At most one approved roster per mission, enforced by a partial unique index.
|
|
/// Two approved proposals are two answers to "what shape is this mission".
|
|
#[tokio::test]
|
|
async fn a_mission_cannot_have_two_approved_rosters() {
|
|
let pool = cm_testkit::test_pool().await;
|
|
let ws = workspace(&pool).await;
|
|
let m = mission(&pool, ws).await;
|
|
|
|
let a = Uuid::now_v7();
|
|
let b = Uuid::now_v7();
|
|
for id in [a, b] {
|
|
proposals::insert(&pool, id, m, ws.as_uuid().to_owned(), &roster(), "claude-opus-4-8")
|
|
.await
|
|
.expect("insert");
|
|
}
|
|
|
|
assert!(proposals::decide(&pool, a, ws.as_uuid().to_owned(), "approved", None, None)
|
|
.await
|
|
.expect("approve a"));
|
|
// The second approval must be REFUSED by the database, not merely lose a
|
|
// race in the handler.
|
|
let second = proposals::decide(&pool, b, ws.as_uuid().to_owned(), "approved", None, None).await;
|
|
assert!(second.is_err(), "a second approved roster was allowed: {second:?}");
|
|
|
|
// Rejecting it is still fine — the constraint is on approvals only, and the
|
|
// ones a human turned down are the record of what the planner gets wrong.
|
|
assert!(proposals::decide(&pool, b, ws.as_uuid().to_owned(), "rejected", Some("too many VMs"), None)
|
|
.await
|
|
.expect("reject b"));
|
|
let rows = proposals::list(&pool, m, ws.as_uuid().to_owned()).await.expect("list");
|
|
assert_eq!(rows.len(), 2, "a rejected proposal is kept, not deleted");
|
|
assert!(rows.iter().any(|r| r.status == "rejected" && r.note.as_deref() == Some("too many VMs")));
|
|
}
|
|
|
|
/// Another workspace's proposal is not visible and not decidable. Every read
|
|
/// here is scoped, and this is the test that keeps it that way.
|
|
#[tokio::test]
|
|
async fn a_proposal_belongs_to_its_workspace() {
|
|
let pool = cm_testkit::test_pool().await;
|
|
let ws = workspace(&pool).await;
|
|
let other = workspace(&pool).await;
|
|
let m = mission(&pool, ws).await;
|
|
let id = Uuid::now_v7();
|
|
proposals::insert(&pool, id, m, ws.as_uuid().to_owned(), &roster(), "claude-opus-4-8")
|
|
.await
|
|
.expect("insert");
|
|
|
|
assert!(proposals::get(&pool, id, other.as_uuid().to_owned()).await.expect("get").is_none());
|
|
assert!(proposals::list(&pool, m, other.as_uuid().to_owned()).await.expect("list").is_empty());
|
|
assert!(
|
|
!proposals::decide(&pool, id, other.as_uuid().to_owned(), "approved", None, None)
|
|
.await
|
|
.expect("decide"),
|
|
"another workspace must not be able to approve this roster"
|
|
);
|
|
}
|
|
|
|
/// The bug production found on the FIRST real approval, in the exact shape it
|
|
/// had: a mission created through the API with no `config` stores jsonb `null`
|
|
/// — a scalar — and `jsonb_set` refuses a scalar with "cannot set path in
|
|
/// scalar". `coalesce` does not help, because that guards SQL NULL and this is a
|
|
/// perfectly good JSON null of the wrong shape.
|
|
#[tokio::test]
|
|
async fn a_roster_applies_to_a_mission_whose_config_is_json_null() {
|
|
let pool = cm_testkit::test_pool().await;
|
|
let ws = workspace(&pool).await;
|
|
let m = mission(&pool, ws).await;
|
|
// Exactly what `POST /api/missions` stores when the body omits `config`.
|
|
sqlx::query("UPDATE missions SET config = 'null'::jsonb WHERE id = $1")
|
|
.bind(m)
|
|
.execute(&pool)
|
|
.await
|
|
.unwrap();
|
|
|
|
let id = Uuid::now_v7();
|
|
proposals::insert(&pool, id, m, ws.as_uuid().to_owned(), &roster(), "claude-opus-4-8")
|
|
.await
|
|
.expect("insert");
|
|
let graph = json!({"kind":"pipeline","nodes":[{"id":"n0","role":"implementer","attrs":{}}],"edges":[]});
|
|
|
|
let applied = proposals::approve_and_apply(
|
|
&pool,
|
|
id,
|
|
m,
|
|
ws.as_uuid().to_owned(),
|
|
&graph,
|
|
None,
|
|
None,
|
|
)
|
|
.await
|
|
.expect("apply");
|
|
assert!(applied);
|
|
|
|
let (engine, nodes): (Option<String>, Option<i32>) = sqlx::query_as(
|
|
"SELECT team_engine, jsonb_array_length(config->'roster'->'nodes') FROM missions WHERE id = $1",
|
|
)
|
|
.bind(m)
|
|
.fetch_one(&pool)
|
|
.await
|
|
.unwrap();
|
|
assert_eq!(engine.as_deref(), Some("composed"));
|
|
assert_eq!(nodes, Some(1), "the roster must actually be on the mission");
|
|
}
|
|
|
|
/// Claim and apply are one decision, so they must commit or fail together. A
|
|
/// proposal marked `approved` against a mission that never received the roster
|
|
/// is permanent: the partial unique index blocks every later approval, and the
|
|
/// mission runs solo while its proposal says otherwise.
|
|
#[tokio::test]
|
|
async fn a_failed_apply_leaves_the_proposal_undecided() {
|
|
let pool = cm_testkit::test_pool().await;
|
|
let ws = workspace(&pool).await;
|
|
let m = mission(&pool, ws).await;
|
|
let id = Uuid::now_v7();
|
|
proposals::insert(&pool, id, m, ws.as_uuid().to_owned(), &roster(), "claude-opus-4-8")
|
|
.await
|
|
.expect("insert");
|
|
|
|
// A mission id that does not exist in this workspace: the apply half matches
|
|
// no row, which is the failure the transaction has to undo.
|
|
let err = proposals::approve_and_apply(
|
|
&pool,
|
|
id,
|
|
Uuid::now_v7(),
|
|
ws.as_uuid().to_owned(),
|
|
&json!({}),
|
|
None,
|
|
None,
|
|
)
|
|
.await;
|
|
assert!(err.is_err(), "applying to a missing mission must fail: {err:?}");
|
|
|
|
let rows = proposals::list(&pool, m, ws.as_uuid().to_owned()).await.expect("list");
|
|
assert_eq!(
|
|
rows[0].status, "proposed",
|
|
"the claim must have been rolled back, or this proposal is stuck approved forever"
|
|
);
|
|
// And it can still be approved properly afterwards.
|
|
let graph = json!({"kind":"pipeline","nodes":[{"id":"n0","role":"implementer","attrs":{}}],"edges":[]});
|
|
assert!(
|
|
proposals::approve_and_apply(&pool, id, m, ws.as_uuid().to_owned(), &graph, None, None)
|
|
.await
|
|
.expect("apply")
|
|
);
|
|
}
|