fix(missions): the first real approval found two bugs the tests could not
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]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
aa470091aa
commit
75d09241fb
@@ -267,47 +267,33 @@ pub async fn decide(
|
||||
return Err(ApiError::BadRequest);
|
||||
}
|
||||
|
||||
// Claim the decision FIRST. The unique index allows one approved proposal
|
||||
// per mission, so this is what makes two approvals race safely: the loser
|
||||
// updates nothing and never touches the mission.
|
||||
let claimed = cm_db::repo::mission_team_proposals::decide(
|
||||
let graph = roster.graph().map_err(|e| {
|
||||
eprintln!("mission {id}: approved roster does not build a graph: {e}");
|
||||
ApiError::Internal
|
||||
})?;
|
||||
// Claiming the proposal and writing the mission are ONE transaction. Doing
|
||||
// them as two statements left the first real approval in production marked
|
||||
// `approved` with nothing written to the mission — and the partial unique
|
||||
// index then makes that permanent, since no other proposal for that mission
|
||||
// can ever be approved.
|
||||
let claimed = cm_db::repo::mission_team_proposals::approve_and_apply(
|
||||
&state.pool,
|
||||
pid,
|
||||
id,
|
||||
ws.as_uuid().to_owned(),
|
||||
"approved",
|
||||
&graph,
|
||||
body.note.as_deref(),
|
||||
Some(user.user_id.as_uuid().to_owned()),
|
||||
)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
eprintln!("mission {id}: could not approve proposal {pid}: {e}");
|
||||
eprintln!("mission {id}: could not apply roster {pid}: {e}");
|
||||
ApiError::Internal
|
||||
})?;
|
||||
if !claimed {
|
||||
return Err(ApiError::BadRequest);
|
||||
}
|
||||
|
||||
let graph = roster.graph().map_err(|e| {
|
||||
eprintln!("mission {id}: approved roster does not build a graph: {e}");
|
||||
ApiError::Internal
|
||||
})?;
|
||||
sqlx::query(
|
||||
"UPDATE missions
|
||||
SET config = jsonb_set(coalesce(config, '{}'::jsonb), '{roster}', $2::jsonb, true),
|
||||
team_engine = 'composed',
|
||||
updated_at = now()
|
||||
WHERE id = $1 AND workspace_id = $3",
|
||||
)
|
||||
.bind(id)
|
||||
.bind(&graph)
|
||||
.bind(ws.as_uuid())
|
||||
.execute(&state.pool)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
eprintln!("mission {id}: could not write the approved roster: {e}");
|
||||
ApiError::Internal
|
||||
})?;
|
||||
|
||||
eprintln!(
|
||||
"mission_roster: mission {id} now runs a {}-node composed graph from proposal {pid}",
|
||||
roster.members.len()
|
||||
|
||||
Reference in New Issue
Block a user