fix(goal): reject ambiguous model checklist results

This commit is contained in:
mateaix 2026-09-14 00:52:24 +08:00
parent 9f89db8262
commit b58fd93430
4 changed files with 57 additions and 6 deletions

View File

@ -60,6 +60,7 @@ public final class GoalCriteriaCodec {
* Merge a per-round verdict delta into the full checklist by id. Criteria
* absent from the delta retain their state unless their pass lacks evidence.
* The criterion text is always kept from the existing item (the verdict never carries text).
* Duplicate verdict ids are ambiguous and rejected instead of taking the last value.
*/
public static List<GoalCriterion> merge(List<GoalCriterion> existing,
List<GoalChecklistVerdict.CriterionVerdict> verdicts) {
@ -70,7 +71,9 @@ public final class GoalCriteriaCodec {
if (verdicts != null) {
for (GoalChecklistVerdict.CriterionVerdict v : verdicts) {
if (v != null && v.id() != null) {
byId.put(v.id(), v);
if (byId.putIfAbsent(v.id(), v) != null) {
throw new IllegalArgumentException("Duplicate criterion verdict id");
}
}
}
}

View File

@ -81,10 +81,8 @@ public class GoalEvaluationService implements Evaluator {
private final ProviderChatModelFactory chatModelFactory;
private final ObjectMapper objectMapper;
private final BeanOutputConverter<GoalCriteriaDraft> draftConverter =
new BeanOutputConverter<>(GoalCriteriaDraft.class);
private final BeanOutputConverter<GoalChecklistVerdict> verdictConverter =
new BeanOutputConverter<>(GoalChecklistVerdict.class);
private final BeanOutputConverter<GoalCriteriaDraft> draftConverter;
private final BeanOutputConverter<GoalChecklistVerdict> verdictConverter;
public GoalEvaluationService(GoalProperties properties,
ModelConfigService modelConfigService,
@ -94,6 +92,13 @@ public class GoalEvaluationService implements Evaluator {
this.modelConfigService = modelConfigService;
this.chatModelFactory = chatModelFactory;
this.objectMapper = objectMapper;
// Preserve converter tolerance for extra fields, but never silently
// choose the last value of an ambiguous model-produced JSON key.
ObjectMapper evaluatorJson = objectMapper.copy()
.disable(com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
.enable(com.fasterxml.jackson.core.JsonParser.Feature.STRICT_DUPLICATE_DETECTION);
this.draftConverter = new BeanOutputConverter<>(GoalCriteriaDraft.class, evaluatorJson);
this.verdictConverter = new BeanOutputConverter<>(GoalChecklistVerdict.class, evaluatorJson);
}
/**
@ -233,7 +238,8 @@ public class GoalEvaluationService implements Evaluator {
+ "Revoke it when such contradictory evidence exists, citing that evidence. "
+ "An attempted action, a goal description or a claim of completion is not proof. "
+ "Newly passed criteria require concrete observable evidence. Return only changed "
+ "criterion verdicts; omitted criteria retain their previous state. Keep evidence concise. "
+ "criterion verdicts; omitted criteria retain their previous state. Return at most one "
+ "verdict per criterion id. Keep evidence concise. "
+ "Output only the requested JSON.";
private String buildUserPrompt(GoalEntity goal,

View File

@ -12,6 +12,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertThrows;
/**
* Pure unit tests for the checklist (de)serialization + merge helpers.
@ -51,6 +52,17 @@ class GoalCriteriaCodecTest {
assertNull(GoalCriteriaCodec.serialize(null, mapper));
}
@Test
void duplicateVerdictIdsAreRejectedInsteadOfLastWriteWinning() {
var existing = List.of(new GoalCriterion("C1", "report", false, ""));
var failed = new GoalChecklistVerdict.CriterionVerdict("C1", false, "missing");
var passed = new GoalChecklistVerdict.CriterionVerdict("C1", true, "claimed written");
assertThrows(IllegalArgumentException.class, () -> GoalCriteriaCodec.merge(existing, List.of(failed, passed)));
assertThrows(IllegalArgumentException.class, () -> GoalCriteriaCodec.merge(existing, List.of(passed, failed)));
assertThrows(IllegalArgumentException.class, () -> GoalCriteriaCodec.merge(existing, List.of(passed, passed)));
assertFalse(existing.getFirst().passed());
}
// ---------- merge ----------
@Test

View File

@ -29,6 +29,7 @@ import java.util.List;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
@ -269,6 +270,35 @@ class GoalEvaluationServiceTest {
assertEquals(1.0, r.score(), 1e-9);
}
@Test
void contradictoryDuplicateVerdictsCannotProduceCompletion() {
stubChatResponse("{\"criterionVerdicts\":["
+ "{\"id\":\"C1\",\"passed\":false,\"evidence\":\"DNS missing\"},"
+ "{\"id\":\"C1\",\"passed\":true,\"evidence\":\"DNS claimed ready\"},"
+ "{\"id\":\"C2\",\"passed\":true,\"evidence\":\"TLS ready\"}],\"summary\":\"done\"}");
GoalEvaluationResult result = svc.evaluate(goalWithCriteria(), List.of(), "finished");
assertFalse(result.completed());
assertEquals(GoalEvaluationResult.DECISION_FALLBACK, result.decision());
assertEquals(1, result.llmCallsConsumed());
assertTrue(result.criterionVerdicts().isEmpty());
}
@Test
void duplicateJsonFieldsAreRejectedInVerdictAndBootstrap() {
for (boolean bootstrap : List.of(false, true)) {
stubChatResponse(bootstrap
? "{\"criteria\":[{\"text\":\"original requirement\",\"text\":\"replacement\"}]}"
: "{\"criterionVerdicts\":[{\"id\":\"C1\",\"passed\":false,\"passed\":true,\"evidence\":\"claim\"},"
+ "{\"id\":\"C2\",\"passed\":true,\"evidence\":\"claim\"}]}");
GoalEvaluationResult result = svc.evaluate(bootstrap ? goal() : goalWithCriteria(), List.of(), "finished");
assertEquals(GoalEvaluationResult.DECISION_FALLBACK, result.decision());
assertFalse(result.completed());
assertEquals(1, result.llmCallsConsumed());
assertTrue(result.criterionVerdicts().isEmpty());
assertNull(result.bootstrapCriteria());
}
}
// ==================== Parser tolerance ====================
@Test