fix(desktop): bound numeric config fields so compression ratios can't go out of range (fixes #65703) - #65748
Conversation
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Fix bounds numeric config fields so compression ratios cannot go out of range (+163 -6). Good input validation improvement.
Reviewed by Hermes Agent
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the generic desktop input path; current main still reproduces the issue at apps/desktop/src/app/settings/config-settings.tsx:145-160.
Problems
apps/desktop/src/app/settings/constants.ts:291givescompression.target_ratioa0–1range, but runtime clamps it to0.10–0.80inagent/context_compressor.py:1291, and the documented range is also0.10–0.80inwebsite/docs/developer-guide/context-compression-and-caching.md:104. The UI should enforce the runtime contract rather than save values that will be normalized later.apps/desktop/src/app/settings/config-settings.tsx:157writes0for an empty field before calling the clamp helper. That bypasses a positive minimum and would remain invalid oncetarget_ratiouses its actual minimum.
Suggested changes
- Use
0.10–0.80forcompression.target_ratio; retain0–1forcompression.threshold. - Clamp the empty-input fallback too, with a regression test for a field with
min > 0.
Automated hermes-sweeper review.
| // hermes_cli/config.py (values are fractions, not percentages). | ||
| export const NUMBER_BOUNDS: Record<string, NumberBounds> = { | ||
| 'compression.threshold': { min: 0, max: 1, step: 0.05 }, | ||
| 'compression.target_ratio': { min: 0, max: 1, step: 0.05 } |
There was a problem hiding this comment.
target_ratio is not valid across the full 0–1 range: ContextCompressor clamps it to 0.10–0.80 (agent/context_compressor.py:1291), matching the documented range. Please set this fallback to { min: 0.1, max: 0.8, step: 0.05 } so desktop persists the runtime-valid value.
| const n = raw === '' ? 0 : Number(raw) | ||
|
|
||
| if (raw === '') { | ||
| onChange(0) |
There was a problem hiding this comment.
This bypasses clampToBounds. Once a field has a positive minimum (including target_ratio after correcting its valid range), clearing it saves invalid 0; clamp this fallback or preserve the unset state.
teknium1
added
the
sweeper:blast-moderate
… go out of range
The desktop settings render every `type: 'number'` config field through one
generic `<Input type="number">` with no min/max/step, so compression.threshold
and compression.target_ratio accept negatives and values above their valid
range.
- Add optional min/max/step to `ConfigFieldSchema` so the backend can declare
bounds; today the desktop supplies them.
- Add a `NUMBER_BOUNDS` map declaring the two compression ratio fields.
target_ratio uses 0.10-0.80, matching the runtime clamp in
agent/context_compressor.py and the documented range; threshold uses 0-1.
- `resolveNumberBounds()` merges schema-supplied bounds (which win) with the
declared fallback; `clampToBounds()` clamps into range.
- The number field applies min/max/step to the input and clamps in onChange,
including the 0 written when the input is cleared: a bare min/max on an HTML
number input only constrains the spinner, not typed values.
Fields without declared bounds resolve to `{}`; undefined min/max/step are
omitted by React, so every other numeric field behaves exactly as before.
Fixes NousResearch#65703
2485528 to
835c86f
Compare
teknium1
added
the
area/compression
SummaryEight PRs are associated with this two-issue complex. #65748 addresses #65703 by enforcing runtime-aligned bounds in the generic Desktop number input; #49514, #51020, #61637, #63886, and #67206 cover distinct or overlapping memory-provider work for #49513, while #62459 and #67209 concern TTS/STT provider discovery. Related pull requests
Duplicates#49514 substantially overlaps the built-in-memory correction in #67206; #51020 is the source implementation incorporated and extended by #67206; #61637's picker half was superseded by #63886 while its curated-panel half landed separately through #64116; #62459 was incorporated into #67209. None duplicates #65748's #65703 fix. Suggested consolidationKeep #65748 open with a salvage path: preserve its generic bounds resolver, runtime-aligned compression ranges, typed-value clamping, and positive-minimum empty-input regression coverage; this follows the contributor keep_open review, whose requested changes are present in the current diff. Treat #49513 as resolved by merged #67206, with closed #49514 and the overlapping #51020/#61637 paths accounted for by the explicit supersession chain; no additional listed PR should be closed as a duplicate of #65748. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I65703(["issue #65703 (open)"])
P65748["PR #65748 (open)"]
P65748 -->|best fix| I65703
class I65703 open
class P65748 open
class P65748 best
class P65748 target
click I65703 "https://github.com/NousResearch/hermes-agent/issues/65703"
click P65748 "https://github.com/NousResearch/hermes-agent/pull/65748"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 8 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 108 kB of PR diffs, 26 kB of issue/PR text, 14 kB of discussion (18 comments), 7 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
5 participants
What does this PR do?
The desktop settings render every
type: 'number'config field through one generic<Input type="number">inconfig-settings.tsxthat carries nomin/max/step. Socompression.thresholdandcompression.target_ratioaccept negatives and values above their valid range, exactly as reported.Repro (code-level, addressing
needs-repro): on currentmainthe number branch is(
apps/desktop/src/app/settings/config-settings.tsx:154-171) — no bound anywhere, so-0.5/1.5flow straight into config for any numeric field.The fix declares bounds per field, applies them to the input, and clamps in
onChange. That second half is what actually enforces the bound: a baremin/maxon an HTML number input only constrains the spinner arrows and native validation, so a user can still type-5or1.5.Bounds mirror the runtime contract, not the nominal
[0, 1]of a ratio:compression.target_ratio→0.10-0.80, matching the runtime clampmax(0.10, min(summary_target_ratio, 0.80))atagent/context_compressor.py:1331and the documented range atwebsite/docs/developer-guide/context-compression-and-caching.md:104. Saving0.05from the UI would otherwise be silently normalized away by the runtime.compression.threshold→0-1(fraction of the context window that triggers compression;hermes_cli/config.py, default0.50).Fields without declared bounds resolve to
{};undefinedmin/max/stepare omitted by React, so every other numeric field renders and behaves exactly as before.Note on the "slider" wording
The issue calls these sliders; the actual control is a number input. The suggested fix is implemented on that control, plus the typed-value clamp so the bound holds regardless of how the value is entered.
Notes for reviewers
main(19527db731) after the previous head went stale, so it is a fresh commit rather than a rebase of the old ones.target_rationow uses the runtime's0.10-0.80rather than0-1, and the0written when the input is cleared is clamped too, with a regression test for a field whosemin > 0.feat/personality-labels-i18n) edits the same 9-line import block inapps/desktop/src/app/settings/helpers.test.ts, about 2 lines from this PR's edit, which is inside git's 3-line context. Whichever of the two lands second will need a trivial rebase of that import list. No pre-merge was attempted.Related Issue
Fixes #65703
Type of Change
Changes Made
apps/desktop/src/types/hermes.ts— add optionalmin/max/steptoConfigFieldSchemaso the backend can declare bounds in future.apps/desktop/src/app/settings/constants.ts— newNumberBoundstype andNUMBER_BOUNDSmap declaringcompression.threshold(0-1, step0.05) andcompression.target_ratio(0.10-0.80, step0.05). Extensible: add a key to bound another numeric field.apps/desktop/src/app/settings/helpers.ts—resolveNumberBounds(schemaKey, schema)merges schema-supplied bounds (which win, when finite) over the declared fallback;clampToBounds(n, bounds)clamps into range.apps/desktop/src/app/settings/config-settings.tsx— the number field appliesmin/max/stepto the input and clamps the value inonChange, including the0fallback written for a cleared input.apps/desktop/src/app/settings/helpers.test.ts— 11 new cases covering both helpers.How to Test
-0.5into Target Ratio, then1.5. Before: both are saved verbatim. After: they clamp to0.10and0.80.compression.thresholdclamps to0/1.0is written, which is out of range. After: it clamps to0.10.memory.memory_char_limit): nomin/max/stepattributes, behavior unchanged.cd apps/desktop && npx vitest run --project ui src/app/settings/helpers.test.ts→ 41 passed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — N/A: this is a TypeScript-only change underapps/desktop, no Python touched. The JS/TS equivalents were run instead, listed below.Verification actually run (from
apps/desktop):npm run typecheck(tsc -p . --noEmit && tsc -p tsconfig.electron.json --noEmit) → clean, exit 0.npx vitest run --project ui src/app/settings/helpers.test.ts→ 41 passed (1 file).npx vitest run --project ui src/app/settings→ 165 passed, 2 failed (18 files).npx vitest run --project ui(full renderer suite) → 1594 passed, 1 skipped, 2 failed (195 files).npx vitest run --project electron→ 458 passed, 1 skipped (45 files).npm run lint→ 0 errors; 9 pre-existingno-restricted-globalswarnings, none in the files touched here.npm run fix(eslint --fix+prettier --write) → produced no changes to the diff.git diff --check→ clean.The 2 failures in both
--project uiruns are the same two, both insrc/app/settings/billing/index.test.tsx(rejects auto-refill amounts outside the billing boundsanddisables buy controls while polling and renders the settled outcome). Confirmed pre-existing by stashing this branch's changes and re-running that file against cleanmain: still 2 failed / 16 passed.Documentation & Housekeeping
docs/, docstrings) — N/A: no new user-facing behavior beyond enforcing an already-documented range.website/docs/developer-guide/context-compression-and-caching.mdalready states0.10-0.80fortarget_ratio; this change makes the UI honor it.cli-config.yaml.exampleif I added/changed config keys — N/A: no config keys added or renamed, only UI-side bounds on existing keys.CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A: no architecture or workflow change.Screenshots / Logs
Not attached: the change is an input constraint, and its effect is shown more precisely by the unit tests above than by a screenshot of a number field.