fix(missions): an unmet goal condition is no longer reported as success
Found by the Goodhart test for the independent judge, which is exactly what it was built to find. The test: a phase whose `done_when` demanded a passing suite, and a task that deliberately left a failing test. glm-4.7 judged it, ran `cargo test` itself, saw `parity_is_wrong_on_purpose ... FAILED` (exit 101), and returned met=false quoting the assertion — while the agent's own summary said "All three steps are implemented exactly as specified and independently verified". The verdict and the agent's account diverged, which is the whole point of an independent judge. And then the mission closed `completed`. `if verdict.met || last_pass` marked BOTH outcomes completed, so a phase that ran out of passes without ever meeting its condition reported success — and through `close_finished_missions`, so did the mission. The verdict said met=false in a column nobody reads before believing a green status. Anything consuming mission status rather than digging into the verdict saw a goal that was never reached as a goal achieved. Exhausted-and-unmet is now `failed`, and the log names the judge and whether it was independent. This changes observable behaviour: missions that would previously have finished green with an unmet condition now finish failed. That is the correction, not a regression — but it is worth knowing before the next scheduled run. Also: `Verdict.independent` had no column. The field existed in the struct and in the logs, so the audit question the mechanism exists to answer — was this checked by something other than the model that wrote it? — could not be asked of the database. Migration 0067 adds it, defaulting to false, which is the truth about every row written before now. Verified in production before the fix: glm-4.7, 4 checks all executed, the real cargo failure quoted, met=false. 475 tests pass, clippy clean. Note for whoever rebases: `sqlx::migrate!` embeds migrations at COMPILE time, so a new migration needs cm-db rebuilt (`touch crates/cm-db/src/lib.rs`) or the integration tests fail on a column that exists in the file and not in the binary.
This commit is contained in:
@@ -679,12 +679,14 @@ pub async fn record(
|
|||||||
) -> Result<(), sqlx::Error> {
|
) -> Result<(), sqlx::Error> {
|
||||||
sqlx::query(
|
sqlx::query(
|
||||||
"INSERT INTO mission_phase_evaluations
|
"INSERT INTO mission_phase_evaluations
|
||||||
(id, mission_id, phase_id, iteration, met, reason, guidance, model, error, checks)
|
(id, mission_id, phase_id, iteration, met, reason, guidance, model, error,
|
||||||
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)
|
checks, independent)
|
||||||
|
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
|
||||||
ON CONFLICT (phase_id, iteration) DO UPDATE
|
ON CONFLICT (phase_id, iteration) DO UPDATE
|
||||||
SET met = EXCLUDED.met, reason = EXCLUDED.reason,
|
SET met = EXCLUDED.met, reason = EXCLUDED.reason,
|
||||||
guidance = EXCLUDED.guidance, model = EXCLUDED.model,
|
guidance = EXCLUDED.guidance, model = EXCLUDED.model,
|
||||||
error = EXCLUDED.error, checks = EXCLUDED.checks",
|
error = EXCLUDED.error, checks = EXCLUDED.checks,
|
||||||
|
independent = EXCLUDED.independent",
|
||||||
)
|
)
|
||||||
.bind(Uuid::now_v7())
|
.bind(Uuid::now_v7())
|
||||||
.bind(mission_id)
|
.bind(mission_id)
|
||||||
@@ -696,6 +698,7 @@ pub async fn record(
|
|||||||
.bind(&v.model)
|
.bind(&v.model)
|
||||||
.bind(v.error.as_deref())
|
.bind(v.error.as_deref())
|
||||||
.bind(serde_json::json!(v.checks))
|
.bind(serde_json::json!(v.checks))
|
||||||
|
.bind(v.independent)
|
||||||
.execute(pool)
|
.execute(pool)
|
||||||
.await
|
.await
|
||||||
.map(|_| ())
|
.map(|_| ())
|
||||||
@@ -781,6 +784,23 @@ mod cross_provider_tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A phase that ran out of passes without meeting its condition did NOT
|
||||||
|
/// succeed. It used to be recorded `completed` alongside a verdict saying
|
||||||
|
/// `met=false`, so mission status reported a goal that was never reached as a
|
||||||
|
/// goal achieved. Found by the Goodhart test: an independent judge refused the
|
||||||
|
/// phase, and the mission closed green anyway.
|
||||||
|
#[test]
|
||||||
|
fn an_unmet_condition_does_not_close_a_phase_as_completed() {
|
||||||
|
// Mirrors the decision in `phase_runner::evaluate_finished_phases`.
|
||||||
|
let outcome = |met: bool| if met { "completed" } else { "failed" };
|
||||||
|
assert_eq!(outcome(true), "completed");
|
||||||
|
assert_eq!(
|
||||||
|
outcome(false),
|
||||||
|
"failed",
|
||||||
|
"an exhausted, unmet phase must not share a status with a met one"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// A verdict that has not been marked independent must not read as one. This
|
/// A verdict that has not been marked independent must not read as one. This
|
||||||
/// is the field's default, and old rows stored before it existed deserialize
|
/// is the field's default, and old rows stored before it existed deserialize
|
||||||
/// to exactly that.
|
/// to exactly that.
|
||||||
|
|||||||
@@ -1020,18 +1020,33 @@ async fn evaluate_finished_phases(
|
|||||||
|
|
||||||
let last_pass = iteration + 1 >= max_iterations;
|
let last_pass = iteration + 1 >= max_iterations;
|
||||||
if verdict.met || last_pass {
|
if verdict.met || last_pass {
|
||||||
|
// A phase that ran out of passes WITHOUT meeting its condition did not
|
||||||
|
// succeed, and must not say it did. This used to mark both outcomes
|
||||||
|
// `completed`: the verdict recorded met=false while the phase — and
|
||||||
|
// through `close_finished_missions`, the whole mission — reported
|
||||||
|
// success. Anything reading mission status rather than digging into the
|
||||||
|
// verdict saw a goal that was never reached as a goal achieved.
|
||||||
|
//
|
||||||
|
// Found by the Goodhart test for the independent judge: glm-4.7
|
||||||
|
// correctly refused a phase whose suite had a failing test, and the
|
||||||
|
// mission still closed `completed`.
|
||||||
|
let status = if verdict.met { "completed" } else { "failed" };
|
||||||
sqlx::query(
|
sqlx::query(
|
||||||
"UPDATE mission_phases SET status = 'completed', completed_at = now()
|
"UPDATE mission_phases SET status = $2, completed_at = now()
|
||||||
WHERE id = $1 AND status = 'evaluating'",
|
WHERE id = $1 AND status = 'evaluating'",
|
||||||
)
|
)
|
||||||
.bind(phase_id)
|
.bind(phase_id)
|
||||||
|
.bind(status)
|
||||||
.execute(pool)
|
.execute(pool)
|
||||||
.await
|
.await
|
||||||
.map_err(|e| format!("complete phase {phase_id}: {e}"))?;
|
.map_err(|e| format!("close phase {phase_id}: {e}"))?;
|
||||||
eprintln!(
|
eprintln!(
|
||||||
"phase_runner: phase {phase_id} ({kind}) completed after {} pass(es) — met={} — {}",
|
"phase_runner: phase {phase_id} ({kind}) {status} after {} pass(es) — met={} \
|
||||||
|
(judge={}, independent={}) — {}",
|
||||||
iteration + 1,
|
iteration + 1,
|
||||||
verdict.met,
|
verdict.met,
|
||||||
|
verdict.model,
|
||||||
|
verdict.independent,
|
||||||
verdict.reason
|
verdict.reason
|
||||||
);
|
);
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -0,0 +1,16 @@
|
|||||||
|
-- Record whether a phase verdict came from a judge INDEPENDENT of the agent that
|
||||||
|
-- did the work — a different provider family, not merely a different model.
|
||||||
|
--
|
||||||
|
-- `Verdict.independent` existed in the struct and in the logs before this column
|
||||||
|
-- did, so the field was a claim held in memory and thrown away. The audit question
|
||||||
|
-- this whole mechanism exists to answer — "was this work checked by something other
|
||||||
|
-- than the model that produced it?" — could not be asked of the database.
|
||||||
|
--
|
||||||
|
-- Defaults to false, which is the truth about every row written before now: the
|
||||||
|
-- judge was Claude and so was the agent.
|
||||||
|
ALTER TABLE mission_phase_evaluations
|
||||||
|
ADD COLUMN IF NOT EXISTS independent boolean NOT NULL DEFAULT false;
|
||||||
|
|
||||||
|
-- Find the verdicts nobody independently checked.
|
||||||
|
CREATE INDEX IF NOT EXISTS mission_phase_evaluations_independent_idx
|
||||||
|
ON mission_phase_evaluations (independent);
|
||||||
Reference in New Issue
Block a user