Skip to content

fix(semantic-consistency): keep a usable reason when the model omits one - #635

Open
ydflow wants to merge 1 commit into
spring-ai-alibaba:mainfrom
ydflow:fix/semantic-consistency-null-reason
Open

ydflow wants to merge 1 commit into
spring-ai-alibaba:mainfrom
ydflow:fix/semantic-consistency-null-reason

Conversation

@ydflow

@ydflow ydflow commented Sep 22, 2026

Copy link
Copy Markdown

What this PR does / why we need it

SemanticConsistencyNode#buildValidationResult forwarded the model's reason straight into the retry state:

return Map.of(SEMANTIC_CONSISTENCY_NODE_OUTPUT, false, SQL_REGENERATE_REASON,
        SqlRetryDto.semantic(validationResult));

SemanticConsistencyOutputDTO#reason is an optional field — a response of {"passed":false} with no reason is entirely legal for a model to emit. In that case the node stored SqlRetryDto(null, true, false).

That null then travels the full retry path:

SemanticConsistencyNode  →  SQL_REGENERATE_REASON (SqlRetryDto.reason = null)
SqlGenerateNode          →  SqlGenerationDTO.exceptionMessage
Nl2SqlServiceImpl       →  buildSqlErrorFixerPrompt(...)
sql-error-fixer.txt     →  "## 错误信息\n\n{error_message}"

so the repair model receives an empty 错误信息 section and is asked to fix a SQL statement with no stated reason. It has to guess, which is exactly the kind of silent quality loss that is hard to attribute later.

Root cause

reason is treated as always-present when it is an optional model output field. The same class of defect was fixed for SqlExecuteNode in a companion PR (there the null leaked into the literal user-facing string SQL执行失败: null); here it leaks into the prompt instead.

The fix

Fall back to a generic notice when the reason is blank:

SqlRetryDto.semantic(StringUtils.isNotBlank(validationResult) ? validationResult
        : "语义一致性校验未通过,模型未给出具体原因")
  • When the model supplies a reason, the stored value is byte-for-byte unchanged.
  • When it does not, the retry prompt still contains an explanation of what failed, so the repair model is not guessing.
  • The verdict (passed=false) and the retry routing (semanticFail=true) are untouched, so the workflow still loops back to SqlGenerateNode.

org.apache.commons.lang3.StringUtils is the same utility already used across the workflow nodes; one import added.

How to verify

RED (before the fix)

mvn -o -pl data-agent-management -am -Dtest=SemanticConsistencyNodeTest -Dsurefire.failIfNoSpecifiedTests=false test

Tests run: 10, Failures: 1, Errors: 0, Skipped: 0
  SemanticConsistencyNodeTest.apply_failedWithoutReason_doesNotStoreNullReason
    reason must not be null; it is rendered into the repair prompt ==> expected: not <null>

The test drives the real node with a genuine {"passed":false} model response (no mocking of BeanOutputConverter), so the null comes from the production path.

GREEN (after the fix)

Same command → Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Full module

mvn -o -pl data-agent-management -am -Dspotless.apply.skip=true test

Tests run: 1714, Failures: 6, Errors: 0, Skipped: 0

The 6 failures (PromptHelperTest 4, NodeTracingLifecycleListenerTest 1, PlannerNodeTest 1) are pre-existing on upstream main — isolated by reverting to the upstream versions of the touched files and re-running those three suites. The count is 1714 rather than 1712 because two open PRs of mine each add one test; on this branch alone the delta is +1.

mvn -o -pl data-agent-management -am -Dspotless.apply.skip=true checkstyle:check → You have 0 Checkstyle violations. BUILD SUCCESS.

Special notes for reviews

  • No new dependency, no API/schema change, no behaviour change when the model supplies a reason.
  • SqlUtil.findGeneratedSqlValidationError structural failures already pass a non-null reason, so the structural path is unaffected.
  • Related, deliberately out of scope: SqlExecuteNode's e.getMessage() handling is fixed in a separate PR (fix(sql-execute): fall back to exception type when error message is null #634) so each change stays reviewable on its own.

SemanticConsistencyNode#buildValidationResult passed the model's reason
straight into SqlRetryDto.semantic(). A response of {"passed":false}
without a "reason" field therefore stored a null reason, which then
reaches SqlGenerationDTO.exceptionMessage and is rendered into the
{error_message} section of the sql-error-fixer prompt — leaving the
repair model with an empty "错误信息" block and nothing to act on.

Fall back to a generic notice when the reason is blank so the retry
prompt always carries an explanation. Behaviour is unchanged whenever
the model supplies a reason.

Add a regression test asserting the stored reason is never null while
the failure verdict and retry routing are preserved.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant