fix(goal): recheck current criteria before automatic completion

This commit is contained in:
mateaix 2026-09-13 21:00:40 +08:00
parent 7d26eaf528
commit 501a1f8bdb
5 changed files with 128 additions and 8 deletions

View File

@ -202,7 +202,7 @@ public class GoalEvaluationNode implements NodeAction {
// Completion is the deterministic "all criteria passed" signal the
// evaluator already folded into result.completed() no score gate.
if (result.completed()) {
GoalEntity completed = goalService.markCompleted(refreshed.getId(), result);
GoalEntity completed = goalService.markEvaluatedCompleted(refreshed.getId(), result);
return MateClawStateAccessor.output()
.goalEvaluationResult(result.toMap())
.goalEvaluatedThisRun(true)

View File

@ -59,6 +59,9 @@ public interface GoalService {
/** Flip active->completed. Writes a 'completed' event. */
GoalEntity markCompleted(Long id, GoalEvaluationResult result);
/** Complete an evaluator result only if the current active checklist still passes with evidence. */
GoalEntity markEvaluatedCompleted(Long id, GoalEvaluationResult result);
/** Flip active->exhausted with the reason that triggered it. */
GoalEntity markExhausted(Long id, String reason);

View File

@ -340,15 +340,37 @@ public class GoalServiceImpl implements GoalService {
@Override
@Transactional
public GoalEntity markCompleted(Long id, GoalEvaluationResult result) {
return completeGoal(id, result, false);
}
@Override
@Transactional
public GoalEntity markEvaluatedCompleted(Long id, GoalEvaluationResult result) {
if (result == null || !result.completed()
|| !GoalEvaluationResult.DECISION_COMPLETED.equals(result.decision())) {
throw new MateClawException("err.goal.completion_not_verified", 409,
"Automatic completion requires a completed evaluation");
}
return completeGoal(id, result, true);
}
private GoalEntity completeGoal(Long id, GoalEvaluationResult result, boolean evaluated) {
GoalEntity g = retryOptimistic(id, "markCompleted", fresh -> {
if (fresh.getStatus().isTerminal()) return null; // idempotent
if (fresh.getStatus().isTerminal()) {
if (evaluated && fresh.getStatus() != GoalStatus.COMPLETED) {
throw new MateClawException("err.goal.completion_not_verified", 409,
"Automatic completion cannot replace another terminal state");
}
return null; // idempotent
}
boolean persistent = Boolean.TRUE.equals(fresh.getPersistentExecution());
List<GoalCriterion> existing = GoalCriteriaCodec.parse(fresh.getCriteria(), objectMapper);
if (persistent && (fresh.getStatus() != GoalStatus.ACTIVE || existing.isEmpty()
|| existing.stream().anyMatch(c -> c == null || !c.passed()
|| c.evidence() == null || c.evidence().isBlank()))) {
// Rechecked against the fresh row on every CAS retry. A stale
// evaluator result must never force-pass newly added criteria.
if ((persistent || evaluated) && (fresh.getStatus() != GoalStatus.ACTIVE
|| !GoalCriteriaCodec.allPassed(existing))) {
throw new MateClawException("err.goal.completion_not_verified", 409,
"Persistent completion requires an active goal and evidence for every current criterion");
"Completion requires an active goal and evidence for every current criterion");
}
LambdaUpdateWrapper<GoalEntity> w = baseLockedUpdate(fresh)
.set(GoalEntity::getStatus, GoalStatus.COMPLETED);
@ -356,9 +378,9 @@ public class GoalServiceImpl implements GoalService {
w.set(GoalEntity::getCompletionScore, result.score())
.set(GoalEntity::getProgressSummary, result.gap());
}
// Preserve verified persistent evidence verbatim. Legacy manual
// Preserve automatically evaluated and persistent evidence verbatim. Legacy manual
// completion retains its historical force-passed checklist snapshot.
if (!persistent && !existing.isEmpty()) {
if (!persistent && !evaluated && !existing.isEmpty()) {
List<GoalCriterion> allPassed = existing.stream()
.map(c -> c.passed() ? c : new GoalCriterion(c.id(), c.text(), true,
c.evidence() == null || c.evidence().isBlank()

View File

@ -215,6 +215,22 @@ class GoalEvaluationNodeContinuationTest {
assertInstanceOf(Map.class, ((List<?>) criteria).get(0));
}
@Test
void automaticCompletionUsesCurrentStateGuardAndDoesNotEmitSuccessOnConflict() throws Exception {
Fixture f = new Fixture();
var completed = new GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0, List.of(), null);
when(f.evaluationService.evaluate(any(), anyList(), anyString())).thenReturn(completed);
when(f.goalService.markEvaluatedCompleted(eq(1L), eq(completed)))
.thenThrow(new vip.mate.exception.MateClawException(409, "current criteria changed"));
var out = f.node().apply(f.state(FinishReason.NORMAL.getValue(), 0, 0));
verify(f.goalService).markEvaluatedCompleted(1L, completed);
verify(f.goalService, never()).markCompleted(any(), any());
@SuppressWarnings("unchecked")
var events = (List<GraphEventPublisher.GraphEvent>) out.get(MateClawStateKeys.PENDING_EVENTS);
assertTrue(events.stream().noneMatch(event -> "goal_completed".equals(event.type())));
assertEquals(Boolean.TRUE, out.get(MateClawStateKeys.GOAL_EVALUATED_THIS_RUN));
}
// ===== Test fixture =====
private static final class Fixture {

View File

@ -315,6 +315,85 @@ class GoalServiceTest {
assertTrue(setsProperty(update.getValue(), "persistentExecution"), update.getValue().getSqlSet());
}
private GoalEvaluationResult completedEvaluation() {
return new GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0,
java.util.List.of(), null);
}
@Test
void automaticCompletionCannotForcePassNewCriteria() {
GoalEntity goal = verifiedPersistentGoal(GoalStatus.ACTIVE);
goal.setPersistentExecution(false);
goal.setCriteria("[{\"id\":\"C1\",\"text\":\"new requirement\",\"passed\":false,\"evidence\":\"\"}]");
when(goalMapper.selectById(1L)).thenReturn(goal);
lenient().when(goalMapper.update(any(), any(LambdaUpdateWrapper.class))).thenReturn(1);
assertEquals(409, assertThrows(MateClawException.class,
() -> service.markEvaluatedCompleted(1L, completedEvaluation())).getCode());
verify(goalMapper, never()).update(any(), any(LambdaUpdateWrapper.class));
}
@Test
void automaticCompletionRechecksCurrentCriteriaAfterCasMiss() {
GoalEntity old = verifiedPersistentGoal(GoalStatus.ACTIVE);
old.setPersistentExecution(false);
GoalEntity fresh = verifiedPersistentGoal(GoalStatus.ACTIVE);
fresh.setPersistentExecution(false);
fresh.setVersion(1);
fresh.setCriteria("[{\"id\":\"C1\",\"text\":\"new requirement\",\"passed\":false,\"evidence\":\"\"}]");
when(goalMapper.selectById(1L)).thenReturn(old, fresh);
when(goalMapper.update(any(), any(LambdaUpdateWrapper.class))).thenReturn(0);
assertEquals(409, assertThrows(MateClawException.class,
() -> service.markEvaluatedCompleted(1L, completedEvaluation())).getCode());
verify(goalMapper, times(1)).update(any(), any(LambdaUpdateWrapper.class));
}
@Test
void automaticCompletionCannotOverridePauseOrAbandonment() {
for (GoalStatus status : java.util.List.of(GoalStatus.PAUSED, GoalStatus.ABANDONED)) {
GoalEntity goal = verifiedPersistentGoal(status);
goal.setPersistentExecution(false);
when(goalMapper.selectById(1L)).thenReturn(goal);
assertEquals(409, assertThrows(MateClawException.class,
() -> service.markEvaluatedCompleted(1L, completedEvaluation())).getCode());
}
verify(goalMapper, never()).update(any(), any(LambdaUpdateWrapper.class));
verify(eventMapper, never()).insert(any(GoalEventEntity.class));
}
@Test
void automaticCompletionPreservesCurrentChecklist() {
GoalEntity goal = verifiedPersistentGoal(GoalStatus.ACTIVE);
goal.setPersistentExecution(false);
when(goalMapper.selectById(1L)).thenReturn(goal, statusFlipped(goal, GoalStatus.COMPLETED));
when(goalMapper.update(any(), any(LambdaUpdateWrapper.class))).thenReturn(1);
assertEquals(GoalStatus.COMPLETED, service.markEvaluatedCompleted(1L, completedEvaluation()).getStatus());
ArgumentCaptor<LambdaUpdateWrapper> update = ArgumentCaptor.forClass(LambdaUpdateWrapper.class);
verify(goalMapper).update(any(), update.capture());
assertFalse(update.getValue().getSqlSet().contains("criteria="));
}
@Test
void automaticCompletionRejectsMissingOrFallbackEvaluation() {
assertEquals(409, assertThrows(MateClawException.class,
() -> service.markEvaluatedCompleted(1L, null)).getCode());
assertEquals(409, assertThrows(MateClawException.class,
() -> service.markEvaluatedCompleted(1L, GoalEvaluationResult.fallback("unavailable"))).getCode());
verify(goalMapper, never()).update(any(), any(LambdaUpdateWrapper.class));
}
@Test
void explicitLegacyCompletionRetainsItsSeparateCompatibilityPath() {
GoalEntity goal = persisted(1L, GoalStatus.ACTIVE);
goal.setPersistentExecution(false);
goal.setCriteria("[{\"id\":\"C1\",\"text\":\"manual check\",\"passed\":false,\"evidence\":\"\"}]");
when(goalMapper.selectById(1L)).thenReturn(goal, statusFlipped(goal, GoalStatus.COMPLETED));
when(goalMapper.update(any(), any(LambdaUpdateWrapper.class))).thenReturn(1);
assertEquals(GoalStatus.COMPLETED, service.markCompleted(1L, completedEvaluation()).getStatus());
ArgumentCaptor<LambdaUpdateWrapper> update = ArgumentCaptor.forClass(LambdaUpdateWrapper.class);
verify(goalMapper).update(any(), update.capture());
assertTrue(update.getValue().getSqlSet().contains("criteria="));
}
@Test
void persistentCompletionRequiresFreshPassedCriteriaWithEvidence() {
GoalEntity goal = persisted(1L, GoalStatus.ACTIVE);