From 501a1f8bdb622dec0aac2e4d2c6c5cabb03ebd59 Mon Sep 17 00:00:00 2001 From: mateaix <7333791@qq.com> Date: Sun, 13 Sep 2026 21:00:40 +0800 Subject: [PATCH] fix(goal): recheck current criteria before automatic completion --- .../agent/graph/node/GoalEvaluationNode.java | 2 +- .../vip/mate/goal/service/GoalService.java | 3 + .../mate/goal/service/GoalServiceImpl.java | 36 +++++++-- .../GoalEvaluationNodeContinuationTest.java | 16 ++++ .../mate/goal/service/GoalServiceTest.java | 79 +++++++++++++++++++ 5 files changed, 128 insertions(+), 8 deletions(-) diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/node/GoalEvaluationNode.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/node/GoalEvaluationNode.java index 841f1816..439bd0a2 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/node/GoalEvaluationNode.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/node/GoalEvaluationNode.java @@ -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) diff --git a/mateclaw-server/src/main/java/vip/mate/goal/service/GoalService.java b/mateclaw-server/src/main/java/vip/mate/goal/service/GoalService.java index d4fc2086..e5fb1eff 100644 --- a/mateclaw-server/src/main/java/vip/mate/goal/service/GoalService.java +++ b/mateclaw-server/src/main/java/vip/mate/goal/service/GoalService.java @@ -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); diff --git a/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java b/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java index b6b76bb2..85a3882e 100644 --- a/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java +++ b/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java @@ -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 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 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 allPassed = existing.stream() .map(c -> c.passed() ? c : new GoalCriterion(c.id(), c.text(), true, c.evidence() == null || c.evidence().isBlank() diff --git a/mateclaw-server/src/test/java/vip/mate/agent/graph/node/GoalEvaluationNodeContinuationTest.java b/mateclaw-server/src/test/java/vip/mate/agent/graph/node/GoalEvaluationNodeContinuationTest.java index eea8a165..3312c8b3 100644 --- a/mateclaw-server/src/test/java/vip/mate/agent/graph/node/GoalEvaluationNodeContinuationTest.java +++ b/mateclaw-server/src/test/java/vip/mate/agent/graph/node/GoalEvaluationNodeContinuationTest.java @@ -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) 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 { diff --git a/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java b/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java index 2f38f134..a17e3092 100644 --- a/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java @@ -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 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 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);