Skip to content

Commit e5c5035

Browse files
fix(mcp): validate_expression refuses a blank expression with the save gate's verdict (#27534)
Fixes #27411 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:MCP 工具 `validate_expression` 收到空串或纯空白表达式时,不再回 `ok: true`,改回 `ok: false`,错误文案与保存闸门(`422 INVALID_METADATA`)同一句。 - 为什么改:agent 先用这个工具校验、再保存;以前工具说"没问题",保存却被拒,两边口径不一致。 - 风险与回滚:只收窄了工具对"空白表达式"的判定,非空表达式的结果不变;回滚即 revert 本 PR(一个文件的工具逻辑加一个测试文件)。 - 先看:`packages/mcp/src/mcp-http-tools.ts` 中 `validate_expression` 处理函数里的 `slot.safeParse(expression)` 分支;其次 `VALIDATE_SITE_MAP` 新增的 `slot` 列。 - 两分钟验证:`pnpm --filter @objectstack/mcp exec vitest run src/mcp-validate-expression.test.ts`,预期 22 个用例全部通过。 - 需要你做的:无,席位落地。 ## What changes `validate_expression` now parses the expression with the same spec input schema the save/build gate parses that slot with, before it runs the formula checks: | `site` | schema (from `@objectstack/spec/shared`) | refusal sentence | |:---|:---|:---| | `formula`, `validation`, `flow_condition` | `EvaluatedExpressionInputSchema` | `EVALUATED_EXPRESSION_SOURCE_REQUIRED` | | `template` | `TemplateExpressionInputSchema` | `TYPED_EXPRESSION_SOURCE_REQUIRED.template` | When the schema refuses, the tool answers `ok: false` and each `errors[]` entry is that schema's own issue: `code` is the issue code (`invalid_union` today, the shape the spec docblock documents for a blank bare string at an evaluated slot) and `message` is the published sentence. No blank rule and no sentence is written in `packages/mcp`. `warnings` is empty, `inferredType` is omitted, and `inScope` still comes back so the agent can correct itself. When the schema accepts, the tool runs `validateExpression` exactly as before. **What was narrowed, and why it is the published rule.** Before this change an empty or whitespace-only expression (`''`, `' '`, a tab or newline mix) read `ok: true` at every site, because `validateExpression` treats a blank source as "not authored" and returns ok. Saving the same expression is refused: the formula `expression`, validation and flow-edge conditions are declared with `EvaluatedExpressionInputSchema`, which refuses a source that is blank after trimming (`packages/spec/src/shared/expression.zod.ts`), and template slots with `TemplateExpressionInputSchema`, which refuses a blank template. The tool now gives that verdict. Nothing else is narrowed: every non-blank string passes both schemas, so its answer is byte-for-byte the old one. ## Tests `packages/mcp/src/mcp-validate-expression.test.ts` adds 14 cases: - `''`, `' '` and a tab and newline mix, at `formula`, `validation` and `flow_condition`: `ok: false`, one error, `code` `invalid_union`, `message` equal to the imported `EVALUATED_EXPRESSION_SOURCE_REQUIRED`, no warnings, no `inferredType`, `inScope` still present. - The same three inputs at `template`: `message` equal to `TYPED_EXPRESSION_SOURCE_REQUIRED.template`. - No `site` given defaults to `formula` and refuses. - Controls: `' record.amount / 100 '` (formula) and `'Hi {{ record.title }}'` (template) still answer `ok: true` with no errors. ## Acceptance notes - The error `code` is the spec schema's zod issue code (`invalid_union`), not a kebab code from `@objectstack/formula`'s refusal vocabulary. That vocabulary is closed and owned by `packages/formula`, and its contract is one code per message template, so reusing `empty-expression` (whose message is different) would break it. Adding a new formula code is outside this card's file surface. If a stable kebab code is wanted for this refusal, that is a formula-package change. - The `template` site uses the general `TemplateExpressionInputSchema` sentence. A few template slots (the notify node's `title` and `message`) refuse with their own sentence that names the key. The tool has no key, so it cannot pick between them; the general sentence is the one the `template` site's default slots use. - PR assignee: the dispatch asked for assignee `sunhaolin` on `pr_create`, but the relay's own row comment (`scripts/pm/fleet-write/ops.mjs`, `pr_create`) says a dev's create carries no `assignees`. This PR was opened without one. The seat decides. ## Verification All readings below were taken at head `bb5eb6dd` (the final commit), except the premise row, which was taken on origin/main before the edit. <details><summary>Premise, measured before the edit on the built packages (origin/main ded67bf)</summary> `validateExpression('value', '', hint)` and `validateExpression('value', ' ', hint)` both answered `{"ok":true,"errors":[],"warnings":[]}`. `EvaluatedExpressionInputSchema.safeParse('')` answered one issue, code `invalid_union`, message `EVALUATED_EXPRESSION_SOURCE_REQUIRED`. `TemplateExpressionInputSchema.safeParse('')` answered one issue, code `invalid_union`, message `TYPED_EXPRESSION_SOURCE_REQUIRED.template`. `'record.amount'` passed both. </details> <details><summary>Ablation (one run, on the committed fix)</summary> `node scripts/ablation-replace.mjs` replaced `const slotParse = slot.safeParse(expression);` with a constant `{ success: true }`: anchor 1 to 0, blob `0015f6b5` to `1609783f`. `src/mcp-validate-expression.test.ts`: 13 failed, 9 passed (22), meaning the 12 blank cases plus the default-site case failed and both controls passed. Restored: blob equals HEAD `0015f6b5`, `git diff HEAD` empty. The test imports the tool from `src` through a relative path, so no `dist` rebuild was part of the path. </details> <details><summary>Package test and typecheck</summary> `pnpm --filter @objectstack/mcp test`: Test Files 45 passed (45), Tests 581 passed (581). `pnpm --filter @objectstack/mcp typecheck`: `tsc --noEmit` clean, and `check:test-typecheck` OK (6 files, 53 errors, 8 pinned signatures held in the ledger, unchanged). Lock VERDICT command-exit 0. </details> <details><summary>Derived gate families</summary> `node scripts/pm/dispatch-gates.mjs --commands` (change set of 3 paths against merge base ded67bf) named 59 families; all 59 ran and exited 0, and `--ran` reconciled 59 derived, 59 run, 0 NOT-MEASURED. Three needed a prerequisite first and were re-run after it: `check:lean-entry-closure` (exit 0 after building `@objectstack/objectql`) `check-plugin-teardown-shape --self-test` (exit 0 after fetching its pinned fixture commit on this shallow clone), and `check:dual-build-cjs-loads` (exit 0 once `check:type-check-debt` had built every package: 112 require entry points across 69 packages load). </details> --- _Generated by [Claude Code](https://claude.ai/code/session_017xRBcScm27Q3R5mkYLPWdB)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 21e5979 commit e5c5035

3 files changed

Lines changed: 100 additions & 8 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/mcp': patch
3+
---
4+
5+
fix(mcp): `validate_expression` refuses a blank or whitespace-only expression with the save gate's verdict
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
The `validate_expression` MCP tool used to answer `ok: true` for an empty or whitespace-only expression, while saving that same expression is refused with `422 INVALID_METADATA` ("An expression in an evaluated slot needs a non-blank `source` …"). An agent that validated first and then saved was told the expression was fine.
11+
12+
The tool now parses the expression with the same spec input schema the save/build gate uses for the slot (`EvaluatedExpressionInputSchema` for the `formula`, `validation` and `flow_condition` sites, `TemplateExpressionInputSchema` for the `template` site) before it runs the formula checks. When that schema refuses, the tool answers `ok: false` with that schema's own issue: the `code` is the schema's issue code and the `message` is the published sentence (`EVALUATED_EXPRESSION_SOURCE_REQUIRED`, or `TYPED_EXPRESSION_SOURCE_REQUIRED.template` for a template). The fields and functions in scope still come back. A non-blank expression gets the same answer as before.

‎packages/mcp/src/mcp-http-tools.ts‎

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ import {
6969
// the `packages/spec` campaign standardised on — already carried in
7070
// `api-surface/shared.json`, so reusing it adds nothing to any public surface.
7171
import { strictUnknownKeyError } from '@objectstack/spec/shared';
72+
// [#27411] The input schemas the save/build gate parses an expression slot
73+
// with. `validate_expression` parses with the SAME schema before it runs the
74+
// formula checks, so a blank source gets the gate's verdict and its published
75+
// sentence, never a second blank rule written here.
76+
import { EvaluatedExpressionInputSchema, TemplateExpressionInputSchema } from '@objectstack/spec/shared';
7277
import {
7378
METADATA_UNAVAILABLE_CODE,
7479
metadataPartialListingSentence,
@@ -481,12 +486,22 @@ function jsonText(value: unknown): string {
481486
* whose fields are flattened to top level; a `template` is a text template.
482487
* Exposing a single friendly `site` keeps the tool aligned with how an author
483488
* thinks about *where* the expression goes.
489+
*
490+
* `slot` is the spec input schema the save/build gate parses that site's slot
491+
* with: an evaluated CEL slot (`EvaluatedExpressionInputSchema` — the formula
492+
* `expression`, validation and flow-edge conditions) or a `{{ }}` template slot
493+
* (`TemplateExpressionInputSchema`). The tool parses with it first, so whatever
494+
* the gate refuses at the envelope level — today a source that is blank after
495+
* trimming — the tool refuses too, with the gate's own issue.
484496
*/
485-
const VALIDATE_SITE_MAP: Record<string, { role: FieldRole; scope: 'record' | 'flattened' }> = {
486-
formula: { role: 'value', scope: 'record' },
487-
validation: { role: 'predicate', scope: 'record' },
488-
flow_condition: { role: 'predicate', scope: 'flattened' },
489-
template: { role: 'template', scope: 'record' },
497+
const VALIDATE_SITE_MAP: Record<
498+
string,
499+
{ role: FieldRole; scope: 'record' | 'flattened'; slot: z.ZodType }
500+
> = {
501+
formula: { role: 'value', scope: 'record', slot: EvaluatedExpressionInputSchema },
502+
validation: { role: 'predicate', scope: 'record', slot: EvaluatedExpressionInputSchema },
503+
flow_condition: { role: 'predicate', scope: 'flattened', slot: EvaluatedExpressionInputSchema },
504+
template: { role: 'template', scope: 'record', slot: TemplateExpressionInputSchema },
490505
};
491506

492507
// ── Tool arguments: an undeclared key is REFUSED, not stripped (#16913) ──────
@@ -828,11 +843,27 @@ export function registerObjectTools(
828843
fields.push(f.name);
829844
if (typeof f?.type === 'string') fieldTypes[f.name] = f.type;
830845
}
831-
const { role, scope } = VALIDATE_SITE_MAP[site ?? 'formula'];
846+
const { role, scope, slot } = VALIDATE_SITE_MAP[site ?? 'formula'];
832847
const hint = { objectName, fields, fieldTypes, scope } as const;
833-
const result = validateExpression(role, expression, hint);
848+
// The gate's envelope verdict first: `validateExpression` treats a
849+
// blank source as "not authored" and answers ok, while the save gate
850+
// refuses it. Each issue is the spec schema's own (code and message),
851+
// so the tool and the gate cannot drift apart on this rule.
852+
const slotParse = slot.safeParse(expression);
853+
const result = slotParse.success
854+
? validateExpression(role, expression, hint)
855+
: {
856+
ok: false,
857+
errors: slotParse.error.issues.map((issue) => ({
858+
source: expression,
859+
message: issue.message,
860+
code: issue.code,
861+
})),
862+
warnings: [],
863+
};
834864
const inScope = introspectScope(role, hint);
835-
const inferredType = role === 'value' ? inferExpressionType(expression, hint) : undefined;
865+
const inferredType =
866+
slotParse.success && role === 'value' ? inferExpressionType(expression, hint) : undefined;
836867
return textResult({
837868
ok: result.ok,
838869
errors: result.errors,

‎packages/mcp/src/mcp-validate-expression.test.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@
22

33
import { describe, it, expect, beforeEach } from 'vitest';
44

5+
import {
6+
EVALUATED_EXPRESSION_SOURCE_REQUIRED,
7+
TYPED_EXPRESSION_SOURCE_REQUIRED,
8+
} from '@objectstack/spec/shared';
9+
510
import { MCPServerRuntime } from './mcp-server-runtime.js';
611
import type { McpDataBridge } from './mcp-http-tools.js';
712

@@ -117,6 +122,50 @@ describe('validate_expression MCP tool (#1928)', () => {
117122
expect(json.result.content[0].text).toMatch(/not found/i);
118123
});
119124

125+
// A blank source is refused by the save/build gate (the spec's evaluated-slot
126+
// and template-slot input schemas). The tool's verdict must be that gate's:
127+
// ok:false, the schema's own issue code, and its published sentence.
128+
describe('a blank or whitespace-only expression gets the save gate verdict', () => {
129+
for (const expression of ['', ' ', '\t\n ']) {
130+
for (const site of ['formula', 'validation', 'flow_condition'] as const) {
131+
it(`refuses ${JSON.stringify(expression)} at site ${site}`, async () => {
132+
const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression, site });
133+
expect(r.ok).toBe(false);
134+
expect(r.errors).toHaveLength(1);
135+
expect(r.errors[0].code).toBe('invalid_union');
136+
expect(r.errors[0].message).toBe(EVALUATED_EXPRESSION_SOURCE_REQUIRED);
137+
expect(r.warnings).toHaveLength(0);
138+
expect(r.inferredType).toBeUndefined();
139+
// The scope context still comes back, so the agent can self-correct.
140+
expect(r.inScope.fields).toContain('amount');
141+
});
142+
}
143+
it(`refuses ${JSON.stringify(expression)} at site template with the template sentence`, async () => {
144+
const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression, site: 'template' });
145+
expect(r.ok).toBe(false);
146+
expect(r.errors).toHaveLength(1);
147+
expect(r.errors[0].code).toBe('invalid_union');
148+
expect(r.errors[0].message).toBe(TYPED_EXPRESSION_SOURCE_REQUIRED.template);
149+
});
150+
}
151+
152+
it('defaults to the formula site when none is given', async () => {
153+
const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: '' });
154+
expect(r.ok).toBe(false);
155+
expect(r.errors[0].message).toBe(EVALUATED_EXPRESSION_SOURCE_REQUIRED);
156+
});
157+
158+
it('keeps the verdict of a non-blank control expression unchanged', async () => {
159+
const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: ' record.amount / 100 ', site: 'formula' });
160+
expect(r.ok).toBe(true);
161+
expect(r.errors).toHaveLength(0);
162+
expect(r.inferredType).toBeDefined();
163+
const t = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'Hi {{ record.title }}', site: 'template' });
164+
expect(t.ok).toBe(true);
165+
expect(t.errors).toHaveLength(0);
166+
});
167+
});
168+
120169
it('refuses system objects (fail-closed guard)', async () => {
121170
const { json } = await call(runtime, toolsCall(1, 'validate_expression', { objectName: 'sys_user', expression: 'record.x > 1' }), bridge);
122171
expect(json.result.isError).toBe(true);

0 commit comments

Comments
 (0)