mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-14 03:33:43 +08:00
fix: fence goal evaluations against definition revisions
This commit is contained in:
parent
5c91dcc234
commit
0c045eaff8
@ -53,6 +53,9 @@ public class GoalEntity {
|
||||
/** Long-form objective. Always non-null but may be short. */
|
||||
private String description;
|
||||
|
||||
/** Advances on evaluation-definition edits, independently of optimistic-lock/usage version. */
|
||||
private long evaluationRevision;
|
||||
|
||||
/** LLM-readable exit criteria; evaluator scores against this. Nullable. */
|
||||
@TableField(value = "exit_criteria", updateStrategy = FieldStrategy.ALWAYS)
|
||||
private String exitCriteria;
|
||||
|
||||
@ -37,7 +37,22 @@ public record GoalEvaluationResult(
|
||||
int llmCallsConsumed,
|
||||
long latencyMs,
|
||||
List<GoalChecklistVerdict.CriterionVerdict> criterionVerdicts,
|
||||
List<GoalCriterion> bootstrapCriteria) {
|
||||
List<GoalCriterion> bootstrapCriteria,
|
||||
long evaluationRevision) {
|
||||
|
||||
/** Compatibility for pre-revision callers: valid only for an unedited definition (revision zero). */
|
||||
public GoalEvaluationResult(double score, String gap, String decision, boolean completed,
|
||||
String evaluatorModel, int llmCallsConsumed, long latencyMs,
|
||||
List<GoalChecklistVerdict.CriterionVerdict> criterionVerdicts, List<GoalCriterion> bootstrapCriteria) {
|
||||
this(score, gap, decision, completed, evaluatorModel, llmCallsConsumed, latencyMs,
|
||||
criterionVerdicts, bootstrapCriteria, 0L);
|
||||
}
|
||||
|
||||
/** Stamp the server-captured revision; the model does not choose this value. */
|
||||
public GoalEvaluationResult withEvaluationRevision(long revision) {
|
||||
return new GoalEvaluationResult(score, gap, decision, completed, evaluatorModel, llmCallsConsumed,
|
||||
latencyMs, criterionVerdicts, bootstrapCriteria, revision);
|
||||
}
|
||||
|
||||
public static final String DECISION_COMPLETED = "completed";
|
||||
public static final String DECISION_CONTINUE = "continue";
|
||||
|
||||
@ -122,6 +122,7 @@ public class GoalEvaluationService implements Evaluator {
|
||||
|
||||
List<GoalCriterion> existing = GoalCriteriaCodec.parse(goal.getCriteria(), objectMapper);
|
||||
boolean bootstrap = existing.isEmpty();
|
||||
long evaluationRevision = goal.getEvaluationRevision();
|
||||
|
||||
long start = System.currentTimeMillis();
|
||||
try {
|
||||
@ -148,16 +149,18 @@ public class GoalEvaluationService implements Evaluator {
|
||||
if (body == null || body.isBlank()) {
|
||||
log.warn("[GoalEvaluation] empty response from evaluator model={}", model.getModelName());
|
||||
// The call was really spent — bill it.
|
||||
return GoalEvaluationResult.fallbackAfterCall("empty_response", model.getModelName(), elapsed);
|
||||
return GoalEvaluationResult.fallbackAfterCall("empty_response", model.getModelName(), elapsed)
|
||||
.withEvaluationRevision(evaluationRevision);
|
||||
}
|
||||
|
||||
return bootstrap
|
||||
return (bootstrap
|
||||
? parseBootstrap(body, model.getModelName(), elapsed)
|
||||
: parseVerdict(body, existing, model.getModelName(), elapsed);
|
||||
: parseVerdict(body, existing, model.getModelName(), elapsed))
|
||||
.withEvaluationRevision(evaluationRevision);
|
||||
} catch (Throwable t) {
|
||||
long elapsed = System.currentTimeMillis() - start;
|
||||
log.warn("[GoalEvaluation] evaluator call failed after {}ms: {}", elapsed, t.toString());
|
||||
return GoalEvaluationResult.fallback("call_failed");
|
||||
return GoalEvaluationResult.fallback("call_failed").withEvaluationRevision(evaluationRevision);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -32,6 +32,7 @@ import java.time.LocalDateTime;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Objects;
|
||||
|
||||
/**
|
||||
* Default implementation. Concurrency safety relies on:
|
||||
@ -259,10 +260,32 @@ public class GoalServiceImpl implements GoalService {
|
||||
if (!changed) {
|
||||
return null; // idempotent no-op
|
||||
}
|
||||
boolean exitsChanged = req.getExitCriteria() != null
|
||||
&& !Objects.equals(req.getExitCriteria(), fresh.getExitCriteria());
|
||||
boolean definitionChanged = exitsChanged
|
||||
|| req.getPersistentExecution() != null
|
||||
&& !Objects.equals(req.getPersistentExecution(), Boolean.TRUE.equals(fresh.getPersistentExecution()))
|
||||
|| req.getTitle() != null && !req.getTitle().isBlank()
|
||||
&& !Objects.equals(req.getTitle().trim(), fresh.getTitle())
|
||||
|| req.getDescription() != null && !Objects.equals(req.getDescription(), fresh.getDescription())
|
||||
|| req.getSuccessCheckPrompt() != null
|
||||
&& !Objects.equals(req.getSuccessCheckPrompt(), fresh.getSuccessCheckPrompt());
|
||||
if (definitionChanged) {
|
||||
// Replacing the free-text exit definition requires a new draft.
|
||||
// Other context edits preserve user criterion text but revoke its old verdicts.
|
||||
String criteria = exitsChanged ? null : GoalCriteriaCodec.serialize(
|
||||
GoalCriteriaCodec.parse(fresh.getCriteria(), objectMapper).stream()
|
||||
.map(c -> new GoalCriterion(c.id(), c.text(), false, "")).toList(), objectMapper);
|
||||
w.set(GoalEntity::getEvaluationRevision, Math.addExact(fresh.getEvaluationRevision(), 1L))
|
||||
.set(GoalEntity::getCriteria, criteria)
|
||||
.set(GoalEntity::getCompletionScore, 0.0)
|
||||
.set(GoalEntity::getProgressSummary, "Goal definition changed; reevaluation required");
|
||||
}
|
||||
bumpVersionAndTime(w);
|
||||
return w;
|
||||
});
|
||||
recordAudit("goal.updated", updated, Map.of("by", username));
|
||||
recordAudit("goal.updated", updated, Map.of("by", username,
|
||||
"evaluationRevision", updated.getEvaluationRevision()));
|
||||
return updated;
|
||||
}
|
||||
|
||||
@ -363,6 +386,10 @@ public class GoalServiceImpl implements GoalService {
|
||||
}
|
||||
return null; // idempotent
|
||||
}
|
||||
if (evaluated && result.evaluationRevision() != fresh.getEvaluationRevision()) {
|
||||
throw new MateClawException("err.goal.completion_not_verified", 409,
|
||||
"Automatic completion requires the current evaluation definition revision");
|
||||
}
|
||||
boolean persistent = Boolean.TRUE.equals(fresh.getPersistentExecution());
|
||||
List<GoalCriterion> existing = GoalCriteriaCodec.parse(fresh.getCriteria(), objectMapper);
|
||||
// Rechecked against the fresh row on every CAS retry. A stale
|
||||
@ -471,8 +498,8 @@ public class GoalServiceImpl implements GoalService {
|
||||
.set(GoalEntity::getLastEvaluationAt, LocalDateTime.now());
|
||||
// Late model results still consume usage, but cannot overwrite a
|
||||
// persistent pause/input boundary established while the call ran.
|
||||
if (result != null && (!Boolean.TRUE.equals(fresh.getPersistentExecution())
|
||||
|| fresh.getStatus() == GoalStatus.ACTIVE)) {
|
||||
if (result != null && result.evaluationRevision() == fresh.getEvaluationRevision()
|
||||
&& (!Boolean.TRUE.equals(fresh.getPersistentExecution()) || fresh.getStatus() == GoalStatus.ACTIVE)) {
|
||||
// Persist the checklist by carrier: bootstrap writes the fresh
|
||||
// draft; verdict merges the per-criterion delta into the
|
||||
// current list (re-read on the locked `fresh` to avoid races).
|
||||
@ -507,6 +534,9 @@ public class GoalServiceImpl implements GoalService {
|
||||
detail.put("decision", result.decision());
|
||||
detail.put("evaluatorScore", result.score());
|
||||
detail.put("evaluatorGap", result.gap());
|
||||
detail.put("evaluatedRevision", result.evaluationRevision());
|
||||
detail.put("currentEvaluationRevision", g.getEvaluationRevision());
|
||||
detail.put("staleEvaluation", result.evaluationRevision() != g.getEvaluationRevision());
|
||||
detail.put("evaluatorModel", result.evaluatorModel());
|
||||
detail.put("latencyMs", result.latencyMs());
|
||||
}
|
||||
|
||||
@ -0,0 +1,2 @@
|
||||
-- Independent revision of the goal evaluation definition; usage/version updates do not advance it.
|
||||
ALTER TABLE mate_agent_goal ADD COLUMN evaluation_revision BIGINT NOT NULL DEFAULT 0;
|
||||
@ -0,0 +1,2 @@
|
||||
-- Independent revision of the goal evaluation definition; usage/version updates do not advance it.
|
||||
ALTER TABLE mate_agent_goal ADD COLUMN evaluation_revision BIGINT NOT NULL DEFAULT 0;
|
||||
@ -0,0 +1,2 @@
|
||||
-- Independent revision of the goal evaluation definition; usage/version updates do not advance it.
|
||||
ALTER TABLE mate_agent_goal ADD COLUMN evaluation_revision BIGINT NOT NULL DEFAULT 0;
|
||||
@ -119,6 +119,85 @@ class GoalPersistenceIntegrationTest {
|
||||
assertThrows(MateClawException.class, () -> goalService.markEvaluatedCompleted(created.getId(), delayed));
|
||||
}
|
||||
|
||||
@Test
|
||||
void replacingExitCriteriaRevokesOldCompletion() {
|
||||
GoalEntity created = goalService.create(req("edited-definition-completion", "report"), "alice");
|
||||
goalService.appendCriterion(created.getId(), "old report", "alice");
|
||||
var passed = new vip.mate.goal.model.GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0,
|
||||
java.util.List.of(new vip.mate.goal.model.GoalChecklistVerdict.CriterionVerdict(
|
||||
"C1", true, "old report written")), null);
|
||||
goalService.recordEvaluation(created.getId(), passed, 1, 1);
|
||||
var edit = new vip.mate.goal.model.GoalUpdateRequest();
|
||||
edit.setExitCriteria("require a different report with an appendix");
|
||||
goalService.update(created.getId(), edit, "alice");
|
||||
assertThrows(MateClawException.class, () -> goalService.markEvaluatedCompleted(created.getId(), passed));
|
||||
assertEquals(0.0, goalService.getById(created.getId()).getCompletionScore());
|
||||
}
|
||||
|
||||
@Test
|
||||
void staleDraftAndVerdictCannotCrossDefinitionRevisionButCurrentOnesCan() {
|
||||
GoalEntity created = goalService.create(req("definition-revision-carriers", "report"), "alice");
|
||||
var oldDraft = new vip.mate.goal.model.GoalEvaluationResult(0.0, "draft", "continue", false,
|
||||
"fixture", 1, 0, java.util.List.of(), java.util.List.of(
|
||||
new vip.mate.goal.model.GoalCriterion("C1", "old report", false, "")));
|
||||
var edit = new vip.mate.goal.model.GoalUpdateRequest(); edit.setExitCriteria("new report");
|
||||
goalService.update(created.getId(), edit, "alice");
|
||||
assertEquals(1L, goalService.getById(created.getId()).getEvaluationRevision());
|
||||
goalService.recordEvaluation(created.getId(), oldDraft, 1, 1);
|
||||
assertEquals(null, goalService.getById(created.getId()).getCriteria());
|
||||
var newDraft = new vip.mate.goal.model.GoalEvaluationResult(0.0, "draft", "continue", false,
|
||||
"fixture", 1, 0, java.util.List.of(), java.util.List.of(
|
||||
new vip.mate.goal.model.GoalCriterion("C1", "new report", false, ""))).withEvaluationRevision(1);
|
||||
goalService.recordEvaluation(created.getId(), newDraft, 1, 1);
|
||||
var oldPass = new vip.mate.goal.model.GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0,
|
||||
java.util.List.of(new vip.mate.goal.model.GoalChecklistVerdict.CriterionVerdict(
|
||||
"C1", true, "old report evidence")), null);
|
||||
goalService.recordEvaluation(created.getId(), oldPass, 1, 1);
|
||||
assertEquals(0.0, goalService.getById(created.getId()).getCompletionScore());
|
||||
var currentPass = new vip.mate.goal.model.GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0,
|
||||
java.util.List.of(new vip.mate.goal.model.GoalChecklistVerdict.CriterionVerdict(
|
||||
"C1", true, "new report evidence")), null).withEvaluationRevision(1);
|
||||
goalService.recordEvaluation(created.getId(), currentPass, 1, 1);
|
||||
// Even after current criteria pass, an old result cannot perform the final transition.
|
||||
assertThrows(MateClawException.class, () -> goalService.markEvaluatedCompleted(created.getId(), oldPass));
|
||||
assertEquals(4, goalService.getById(created.getId()).getEvalLlmCallsUsed());
|
||||
assertEquals(GoalStatus.COMPLETED, goalService.markEvaluatedCompleted(created.getId(), currentPass).getStatus());
|
||||
}
|
||||
|
||||
@Test
|
||||
void definitionRevisionSurvivesAbaAndIgnoresIdenticalAndBudgetEdits() {
|
||||
var request = req("definition-revision-aba", "report"); request.setExitCriteria("A");
|
||||
GoalEntity created = goalService.create(request, "alice");
|
||||
var edit = new vip.mate.goal.model.GoalUpdateRequest(); edit.setExitCriteria("A"); edit.setTurnBudget(12);
|
||||
goalService.update(created.getId(), edit, "alice");
|
||||
assertEquals(0L, goalService.getById(created.getId()).getEvaluationRevision());
|
||||
edit.setExitCriteria("B"); goalService.update(created.getId(), edit, "alice");
|
||||
edit.setExitCriteria("A"); goalService.update(created.getId(), edit, "alice");
|
||||
assertEquals(2L, goalService.getById(created.getId()).getEvaluationRevision());
|
||||
var stale = new vip.mate.goal.model.GoalEvaluationResult(0.0, "draft", "continue", false,
|
||||
"fixture", 1, 0, java.util.List.of(), java.util.List.of(
|
||||
new vip.mate.goal.model.GoalCriterion("C1", "A", false, "")));
|
||||
goalService.recordEvaluation(created.getId(), stale, 0, 1);
|
||||
assertEquals(null, goalService.getById(created.getId()).getCriteria());
|
||||
}
|
||||
|
||||
@Test
|
||||
void contextEditPreservesCriterionTextButRevokesPriorPass() {
|
||||
GoalEntity created = goalService.create(req("definition-context-edit", "report"), "alice");
|
||||
goalService.appendCriterion(created.getId(), "user criterion", "alice");
|
||||
var passed = new vip.mate.goal.model.GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0,
|
||||
java.util.List.of(new vip.mate.goal.model.GoalChecklistVerdict.CriterionVerdict(
|
||||
"C1", true, "old evidence")), null);
|
||||
goalService.recordEvaluation(created.getId(), passed, 0, 1);
|
||||
var edit = new vip.mate.goal.model.GoalUpdateRequest(); edit.setDescription("changed context");
|
||||
var saved = goalService.update(created.getId(), edit, "alice");
|
||||
var criteria = vip.mate.goal.model.GoalCriteriaCodec.parse(saved.getCriteria(), new com.fasterxml.jackson.databind.ObjectMapper());
|
||||
assertEquals("user criterion", criteria.getFirst().text());
|
||||
org.junit.jupiter.api.Assertions.assertFalse(criteria.getFirst().passed());
|
||||
assertEquals("", criteria.getFirst().evidence());
|
||||
assertEquals(1L, saved.getEvaluationRevision());
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("GoalStatus values persist as lowercase literals — load-bearing for uk_agent_goal_active_conv")
|
||||
void status_persistsAsLowercaseString() {
|
||||
|
||||
@ -108,6 +108,19 @@ class GoalEvaluationServiceTest {
|
||||
assertTrue(result.gap().contains("TLS enabled"));
|
||||
}
|
||||
|
||||
@Test
|
||||
void stampsRevisionCapturedBeforeTheModelCall() {
|
||||
GoalEntity goal = goalWithCriteria(); goal.setEvaluationRevision(7L);
|
||||
when(modelConfigService.getDefaultModel()).thenReturn(model("fixture"));
|
||||
when(chatModelFactory.buildFor(any(ModelConfigEntity.class), any(RetryTemplate.class))).thenReturn(chatModel);
|
||||
when(chatModel.call(any(Prompt.class))).thenAnswer(call -> {
|
||||
goal.setEvaluationRevision(8L);
|
||||
return new ChatResponse(List.of(new Generation(new AssistantMessage(
|
||||
"{\"criterionVerdicts\":[],\"summary\":\"unchanged\"}"))));
|
||||
});
|
||||
assertEquals(7L, svc.evaluate(goal, List.of(), "answer").evaluationRevision());
|
||||
}
|
||||
|
||||
// ==================== Pre-flight guards ====================
|
||||
|
||||
@Test
|
||||
|
||||
@ -690,6 +690,25 @@ class GoalServiceTest {
|
||||
assertTrue(retry.getParamNameValuePairs().containsValue("Still missing: appendix"));
|
||||
}
|
||||
|
||||
@Test
|
||||
void staleEvaluationStopsProjectingAfterCasRevisionChange() {
|
||||
GoalEntity original = persisted(1L, GoalStatus.ACTIVE);
|
||||
original.setCriteria("[{\"id\":\"C1\",\"text\":\"report\",\"passed\":false,\"evidence\":\"\"}]");
|
||||
GoalEntity fresh = persisted(1L, GoalStatus.ACTIVE);
|
||||
fresh.setVersion(1); fresh.setEvaluationRevision(1L); fresh.setCriteria(original.getCriteria());
|
||||
when(goalMapper.selectById(1L)).thenReturn(original, original, fresh, fresh);
|
||||
when(goalMapper.update(any(), any(LambdaUpdateWrapper.class))).thenReturn(0, 1);
|
||||
var result = new GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0,
|
||||
java.util.List.of(new vip.mate.goal.model.GoalChecklistVerdict.CriterionVerdict("C1", true, "evidence")), null);
|
||||
service.recordEvaluation(1L, result, 2, 1);
|
||||
ArgumentCaptor<LambdaUpdateWrapper> writes = ArgumentCaptor.forClass(LambdaUpdateWrapper.class);
|
||||
verify(goalMapper, times(2)).update(any(), writes.capture());
|
||||
assertTrue(setsProperty(writes.getAllValues().getFirst(), "criteria"));
|
||||
assertFalse(setsProperty(writes.getAllValues().getLast(), "criteria"));
|
||||
assertFalse(setsProperty(writes.getAllValues().getLast(), "completionScore"));
|
||||
assertTrue(writes.getAllValues().getLast().getSqlSet().contains("eval_llm_calls_used = eval_llm_calls_used + 1"));
|
||||
}
|
||||
|
||||
// ==================== criteria checklist ====================
|
||||
|
||||
@Test
|
||||
|
||||
Loading…
Reference in New Issue
Block a user