fix(config): include numeric bounds in schema validation errors#73220
fix(config): include numeric bounds in schema validation errors#73220alchip wants to merge 1 commit into
Conversation
Greptile SummaryThis PR appends explicit numeric bound hints ( Confidence Score: 4/5Safe to merge; the only finding is latent and does not affect any current schema. A single P2 finding — the missing src/config/validation.ts — Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/config/validation.ts
Line: 492-504
Comment:
**Missing `inclusive` check produces misleading bound hint**
Zod attaches an `inclusive` boolean to every `too_big` / `too_small` issue (`true` for `.max()`/`.min()`, `false` for `.lt()`/`.gt()`). When `inclusive` is `false`, the constraint is a strict inequality, so printing "maximum allowed value is N" is wrong — N itself is not allowed. All current config schemas happen to use inclusive bounds, so this is latent, but a future `.lt()` or `.gt()` addition would silently surface incorrect guidance.
```ts
if (record.code === "too_big") {
const maximum = typeof record.maximum === "number" ? record.maximum : null;
const inclusive = record.inclusive !== false; // default to inclusive if absent
if (maximum !== null) {
return inclusive
? `${message} (maximum allowed value is ${maximum})`
: `${message} (value must be less than ${maximum})`;
}
}
if (record.code === "too_small") {
const minimum = typeof record.minimum === "number" ? record.minimum : null;
const inclusive = record.inclusive !== false;
if (minimum !== null) {
return inclusive
? `${message} (minimum allowed value is ${minimum})`
: `${message} (value must be greater than ${minimum})`;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(config): append numeric bounds in va..." | Re-trigger Greptile |
| if (record.code === "too_big") { | ||
| const maximum = typeof record.maximum === "number" ? record.maximum : null; | ||
| if (maximum !== null) { | ||
| return `${message} (maximum allowed value is ${maximum})`; | ||
| } | ||
| } | ||
|
|
||
| if (record.code === "too_small") { | ||
| const minimum = typeof record.minimum === "number" ? record.minimum : null; | ||
| if (minimum !== null) { | ||
| return `${message} (minimum allowed value is ${minimum})`; | ||
| } | ||
| } |
There was a problem hiding this comment.
inclusive check produces misleading bound hint
Zod attaches an inclusive boolean to every too_big / too_small issue (true for .max()/.min(), false for .lt()/.gt()). When inclusive is false, the constraint is a strict inequality, so printing "maximum allowed value is N" is wrong — N itself is not allowed. All current config schemas happen to use inclusive bounds, so this is latent, but a future .lt() or .gt() addition would silently surface incorrect guidance.
if (record.code === "too_big") {
const maximum = typeof record.maximum === "number" ? record.maximum : null;
const inclusive = record.inclusive !== false; // default to inclusive if absent
if (maximum !== null) {
return inclusive
? `${message} (maximum allowed value is ${maximum})`
: `${message} (value must be less than ${maximum})`;
}
}
if (record.code === "too_small") {
const minimum = typeof record.minimum === "number" ? record.minimum : null;
const inclusive = record.inclusive !== false;
if (minimum !== null) {
return inclusive
? `${message} (minimum allowed value is ${minimum})`
: `${message} (value must be greater than ${minimum})`;
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/validation.ts
Line: 492-504
Comment:
**Missing `inclusive` check produces misleading bound hint**
Zod attaches an `inclusive` boolean to every `too_big` / `too_small` issue (`true` for `.max()`/`.min()`, `false` for `.lt()`/`.gt()`). When `inclusive` is `false`, the constraint is a strict inequality, so printing "maximum allowed value is N" is wrong — N itself is not allowed. All current config schemas happen to use inclusive bounds, so this is latent, but a future `.lt()` or `.gt()` addition would silently surface incorrect guidance.
```ts
if (record.code === "too_big") {
const maximum = typeof record.maximum === "number" ? record.maximum : null;
const inclusive = record.inclusive !== false; // default to inclusive if absent
if (maximum !== null) {
return inclusive
? `${message} (maximum allowed value is ${maximum})`
: `${message} (value must be less than ${maximum})`;
}
}
if (record.code === "too_small") {
const minimum = typeof record.minimum === "number" ? record.minimum : null;
const inclusive = record.inclusive !== false;
if (minimum !== null) {
return inclusive
? `${message} (minimum allowed value is ${minimum})`
: `${message} (value must be greater than ${minimum})`;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.|
Gentle bump 🙏\n\nCould a maintainer please help rerun the failed CI jobs for this PR? If rerun is green, happy to proceed toward merge. Thanks! |
|
Rerun still failed on my side, sharing a quick summary for maintainer triage:\n\n- Run: https://github.com/openclaw/openclaw/actions/runs/25032385886\n- Failed job 1: → failed at step \n - Job: https://github.com/openclaw/openclaw/actions/runs/25032385886/job/73316744864\n- Failed job 2: → failed at step \n - Job: https://github.com/openclaw/openclaw/actions/runs/25032385886/job/73316932945\n\n@maintainer Could you please help investigate / rerun from maintainer side? Thank you 🙏 |
|
Correction (proper formatting): rerun is still failing; summary below for maintainer triage.
@maintainer Could you please help investigate / rerun from maintainer side? Thank you 🙏 |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Source inspection shows Next step before merge Security Review findings
Review detailsBest possible solution: Land a narrow mapper change that enriches only numeric-origin bounds, handles inclusive and exclusive constraints accurately, preserves allowed-values behavior, adds focused regression coverage, and records the diagnostics fix in Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows Is this the best way to solve the issue? No, not as submitted. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 23e0be355ac3. |
|
Friendly bump 🙏 Could a maintainer help take a look when time permits? Happy to adjust anything needed. |
|
Closing this for now to reduce queue noise; can reopen or resubmit if needed. |
Summary
too_bigandtoo_smallissuesmaximum allowed value is 5session.agentToAgent.maxPingPongTurnsTesting
Fixes #52500