Skip to content

fix(bot-mode): pin the stranded-reply harvest window to the turn hard cap (contract + coverage) - #113313

Closed
Finn763 wants to merge 1 commit into
NousResearch:mainfrom
Finn763:fix/105247-descr
Closed

Finn763 wants to merge 1 commit into
NousResearch:mainfrom
Finn763:fix/105247-descr

Conversation

@Finn763

@Finn763 Finn763 commented Sep 16, 2026

Copy link
Copy Markdown

Read this first: the behaviour the issue reports is already fixed at HEAD

The 60-tick / 5-minute harvest window that loses late replies is gone on this base: upstream e80642df6003 ("keep group follow-ups ordered and late answers visible") replaced HARVEST_MAX_TRIES = 60 with an inline bound derived from the turn hard cap — Math.ceil((GROUP_TURN_HARD_CAP_MS + 60000) / HARVEST_INTERVAL_MS) ≈ 252 ticks ≈ 21 min. That commit itself cites the harvest-budget approach from #107193, so the fix for the reported symptom is in. On a pristine HEAD the behaviour tests are green; only a test for the not-yet-extracted helper is red.

So this PR is not a behaviour flip — and it does not pretend to be one. What was genuinely missing:

  1. The derivation lived inline at the call site, away from the constant it tracks. It now lives with the cap: groupStrandedHarvestMaxTries(intervalMs) = max(60, ceil((hardCap + 60s) / interval)), exported from group-turns.ts (where GROUP_TURN_HARD_CAP_MS is declared) and consumed by the harvest watcher in group-rounds.ts. Window and cap can no longer drift apart silently — which is precisely how the original 5-minute value survived against a 20-minute cap in the first place.
  2. The reported path had no regression coverage. group-rounds.harvest-window.test.ts pins a busy member whose reply lands past the old five minutes (the reported loss), the new bound (watch length ≥ hard cap), and the stopping conditions. The strand marker mechanism and every non-stranded path are untouched; no scheduler was added.

Evidence

RED → GREEN (my own sabotage, not the author's): forcing the helper back to the issue's old value (return 60) fails 3 of 94:

× watches a busy member long enough to deliver a reply that lands past the old five minutes
    AssertionError: expected 60 to be 61
× bounds the watch at the turn hard cap, then stops instead of polling on
    AssertionError: expected 300000 to be greater than or equal to 1200000
× derives the watch length from the turn hard cap
    AssertionError: expected 300000 to be greater than or equal to 1200000

With the delivery assertion held back, the reported symptom fails verbatim too (expect(await watch.tryTick()).toBe(true) → expected false to be true — the watch had already given up, so the late reply never reaches the room log). Restored → 94/94 passed.

The in-window protection case (a reply inside five minutes is delivered exactly once) stays green under sabotage. No unstoppable loop: the bounded-watch case throws the harvest watch never stops past 1000 ticks and then asserts watch.pending()).toBe(0); the settled/consumed case asserts no timer is left scheduled; a room with nothing stranded asserts the watch was never armed. tsc --noEmit: 0 errors under src/plugins/hermes-bots.

(The desktop node_modules junction was repointed at a sibling tree's real install for these runs and restored to its original target; the worktree is clean.)

Refs #105247.

Not done

@Finn763

Finn763 commented Sep 16, 2026

Copy link
Copy Markdown
Author

Closing: this branch already exists from my earlier attempt (PR #107193, 0ad2d85300f, 2026-09-10) and the change it carries was superseded upstream — @teknium1 landed the cap-derived harvest approach in #111283 (9326d9c) and closed #107193 as superseded. My push was rejected as non-fast-forward, so this PR's head is that older commit while its body described newer work; filing it was a mistake on my side. Nothing to review here.

@Finn763 Finn763 closed this Sep 16, 2026
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants