Repository navigation
feat(desktop): drive Bot Mode room limits from config.yaml - #98616
andredezzy wants to merge 1 commit into
Conversation
Overall: Drives Bot Mode room budgets from Correctness:
Non-blocking nit:
Verdict: LGTM. |
d9e87a8 to
80cfc66
Compare
|
Rebased onto current The case for it is unchanged, and Rechecked on
Two decisions worth a reviewer's attention, since both are judgment rather than mechanism. An unusable value (zero, negative, unparseable) reads as "unset" rather than "stop", because a config typo silencing your bots is worse than ignoring the line. And the ceilings are snapshotted once per drive, so a budget changed mid-drive cannot make "capped" mean two different things in one activity line. CI has never run here either — the workflow runs on this branch are |
Comment on #98616Downstream confirmation that this is the right fix. These caps have been raised at bundle level with a reapply watchdog, documented at https://github.com/ahrazzle/hermes-desktop-mods. This PR retires that patcher. One interaction with #111283: that PR serializes follow-ups behind the active drive, and queued work consumes rounds. The queue makes higher caps more useful and the ceilings more load-bearing.
|
|
Merged current Verified on the merge: typecheck clean, 627 bot-mode tests passing, lint at 243 warnings / 0 errors — identical to @Enough1122 on the observability nit: a silent fallback is the part I'd want to hear about too, since a mistyped ceiling looks exactly like a working one. I left it out of this PR to keep the diff to the config seam; happy to add the warning here or as a follow-up, whichever you prefer. @ahrazzle — good to know the bundle-level patcher covers the same ground. On the interaction with #111283: queued follow-ups consuming rounds is the case where a compiled-in ceiling hurts most, since the right number stops being a constant and starts depending on how a given room is used. Routing stays out of this branch deliberately — #98610 owns that policy, and the two are independent. |
|
Here — I'd put it in this PR. The fallback lives in Minimal scope suggestion: warn only when an explicit configured value fails validation (NaN/Inf/<1 → fallback), not when the key is simply absent — that keeps default-without-config silent and makes the warning actually point at typos, which is the case worth hearing about. With that in, my verdict is unchanged: LGTM. Your rebase validation (typecheck clean, 627 bot-mode tests, lint parity with main) covers what I'd want after a 1444-commit jump.
|
ahrazzle
left a comment
There was a problem hiding this comment.
Scope: limit loading in plugin.tsx and resolver in group-chat.ts at 0f09817. No checks reported. Merge state DIRTY and CONFLICTING.
plugin.tsx:111 loads limits once at plugin start. No other refresh caller appears in this diff. group-chat.ts:1269-1270 says a config edit reaches a room on its next send. That holds after a restart, not for an edit while Desktop stays open. Recommend wiring the existing config change path or narrowing the comment to load time. Maintainer call which one fits this PR.
group-chat.ts:1250 falls back silently. Recommend warning on an explicitly set invalid value only. Keep an absent key silent. This matches the thread suggestion.
Local evidence: group-rounds.test.ts 60 passed. hermes-bots 66 files, 627 passed. tsc renderer 0 errors, electron 0 errors.
Review limit: changed group-chat paths only. No certification of unrelated files.
Automated posting by agentic team with human oversight.
| // than per drive: the round loop is hot, and a gateway round-trip inside it | ||
| // would be a stall the user feels. Failure is silent by design — the | ||
| // shipped defaults already work, so a room still runs. | ||
| void refreshGroupChatLimits() |
There was a problem hiding this comment.
plugin.tsx:111 loads limits once at plugin start. No config change subscription appears in this diff. group-chat.ts:1269-1270 says a config edit reaches a room on its next send. That holds after a restart, not for an edit while Desktop stays open. Recommend using the existing config change path or narrowing the comment to load time.
| function resolveLimit(raw: unknown, fallback: number, hardCap: number): number { | ||
| const value = typeof raw === 'number' || (typeof raw === 'string' && raw.trim()) ? Number(raw) : Number.NaN | ||
|
|
||
| if (!Number.isFinite(value) || value < 1) { |
There was a problem hiding this comment.
resolveLimit falls back silently here. Recommend warning on an explicitly set invalid value only. Keep an absent key silent so default installs stay quiet.
A room's spend ceilings are compiled in: GROUP_CHAT_MAX_ROUNDS, MAX_MESSAGES and MAX_CONTINUATIONS. An operator who wants a longer conversation, or a tighter budget on an expensive model, has to edit the source and rebuild. The repository's own rule puts behavioural settings in config.yaml. A `group_chat` block now supplies the three ceilings, clamped to hard caps so config can raise a budget but never uncap a room. The shipped values stay the defaults, so an install with no `group_chat` block behaves exactly as before. Reading a config that cannot be spent as a limit would be worse than ignoring it, so an unusable value reads as "unset" rather than "stop": zero or negative would mute the room, and a typo should not be what silences your bots. Numeric strings count, since YAML quoting is a formatting choice and `max_rounds: "12"` plainly means twelve. The round loop snapshots the ceilings once per drive rather than reading them per round: a drive that changed its own budget halfway through would make "capped" mean two different things in one activity line. A config edit reaches a room on its next send.
0f09817 to
d0317dd
Compare
Replaces #98047, which accumulated unrelated files after a
mainmerge. This branch isolates the room-limit change.The problem
Bot Mode room limits are hardcoded:
Three rounds is enough for a quick exchange and far too few for a room where specialists hand work to each other. A five-bot room routinely spends its first round just deciding who owns the task, leaving two rounds for the work itself, and the cap lands mid-conversation with no way for the user to raise it.
The repository rubric already says where this belongs: "All behavioral settings — timeouts, thresholds, feature flags — go in
config.yaml." These are thresholds.The change
The three limits read from
group_chatinconfig.yaml, falling back to today's values when unset, so existing installs are unaffected:Resolution goes through one function rather than three call sites, so the precedence rule lives in a single place. Values remain bounded by hard ceilings of 20 rounds, 100 messages, and 20 continuations; this PR does not implement unlimited rooms. Invalid or missing values fall back rather than throwing — a malformed config should not take the room down.
Tests
Cover defaults when unset, override when set, and fallback on invalid input.
Verified against
origin/mainwith no other local changes.