Skip to content

host: a memory review that keeps failing at the same lines calls the model on every turn end and never backs off #2561

Closed

Description

@HugoXK

Implementation: FlyBot (ours) — PR follows within 24h. Do not pick up without a pickup-ok comment on this issue.

Written-by: FlyBot Lead
Claimed-by: FlyBot Lead
Claim-Id: 2e32a352fa474af8bf3576c4d2eaf677
Branch: dev/5-memory-review-backoff
Base: 50df2d3
At: 2026-11-06T01:56:13Z

Implementation: FlyBot (ours) — PR follows within 24h. Do not pick up without a pickup-ok comment.

Written-by: FlyBot Lead

What happens

core/src/host/memory.js: when a memory review fails, reviewSession leaves the cursor where it was (:1510-1516 saves it only on success) and queueReview's catch does nothing but rt.warn (:1557-1560). Every turn end emits agent/status idle, which calls reviewSession again with the same cursor, so the same user lines are handed to the model again — with nothing bounding the repeats. A model that keeps refusing one answer ("the model's answer is not a list of changes: …") therefore costs one model call per turn end for as long as the Session lives, with only log lines to show for it.

REVIEW_PASSES_MAX (:67) and the overflowed map only split one answer into batches; neither looks at failures across calls.

Reproduction (red first)

On unmodified 50df2d3869d56e722e67592cb8bbb24bca6a0732, with only the new test file lines applied, TZ=UTC node --test core/test/memory.test.js → exit 1:

not ok 1 - a review that keeps failing at the same lines gives up after two tries and warns once
  location: 'core/test/memory.test.js:812:1'
  error: Expected values to be strictly equal: 4 !== 2        (strictEqual at :826)

not ok 2 - a success clears the backoff, so a later failure still gets its two tries
  location: 'core/test/memory.test.js:842:1'
  error: Cannot read properties of undefined (reading 'ok')   (assert at :870)
# tests 3 / # pass 1 / # fail 2

The first red is the defect itself: four turn ends, four model requests (the fixture's model always answers the same unusable line). The second red is the missing lastReview field; after the fix it also guards the reset — without a reset on success, the failure after a success would be swallowed and the run would make 3 requests instead of 4.

Expected

  • Two failures at one cursor stop the retries: further turn ends make no model call for those lines, and nothing is passed unseen (the cursor stays put).
  • The stop is warned once, not on every turn end.
  • It is not a latch: a review that succeeds, or a line the user writes afterwards, starts it over, so a Session cannot be blocked for good.
  • memory-view reports the last review as { at, ok, reason } so a panel can say it failed and why; nothing draws it yet.

Scope

  • core/src/host/memory.js — REVIEW_FAILURES_MAX = 2, a per-cursor failure record, an in-flight attempt record, and lastReview on the memory-view payload. The cursor's own behaviour is unchanged.
  • core/test/memory.test.js — three cases: gives up after two tries and warns once; a success clears the backoff; a new user turn clears it. One existing case ("malformed change, an unlisted entry, or a bad topic") used to spend four failing answers at a single cursor, so each bad answer now begins with its own user line; what it checks (a failed answer writes nothing, the cursor stays) is unchanged and only the final cursor reading moves from 11 to 13 with the added lines.
  • patches/ds-bot/0001-core-fixes.patch — regenerated.
  • changelog.d/ fragment. Docs-not-needed: no architecture change: one branch of the review's own retry handling moves, and the modules, the files' shape and what the subsystem page describes are unchanged.

Out of scope: what the panel draws for a failed review (P-35), and the other answer shapes that throw whole (badTopic, secrets) — they keep today's behaviour.

Checked before opening: no open or closed issue covers this surface — REVIEW_FAILURES_MAX does not exist on main (code search, 0 hits); "memory review" in issues (46) has the nearest neighbours at #2558/#2559 (P-600: one unfit entry discards the answer, a different branch of the same function) and #485 (Reflexion: learning from repeated failures, a feature); titles matching retry (28) and backs off/cursor (0/2) are all closed and unrelated.

Activity

  1. HugoXK commented on Nov 6, 2026

    @HugoXK
    ContributorAuthor

    Release

    Written-by: FlyBot Lead
    Released: 2e32a352fa474af8bf3576c4d2eaf677
    Reason: merged as #2562 (e89344a)
    At: 2026-11-06T02:16:17Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions