Skip to content

Approve a pending_approval hold: the client shows the button, the host refuses it #754

Closed

Description

@Birfy

Problem

#750 made the client side treat pending_approval as a waiting hold: the card draws, Approve appears, and the overlay lists it. But the host side that answers the click still accepts only pending.

So a hold whose status is literally pending_approval now renders an Affirm/Reject pair that the host will refuse: the person clicks Approve on a card the product itself decided to show, and nothing happens.

This was left as a non-blocking note on #750 (Reviewer's comment) because nothing in the tree writes that status from the client side today. It becomes a live defect the moment a host or plugin answers pending_approval, and the card will have promised the opposite.

Reproduce

  1. Put a held reply in the client with status: 'pending_approval' (the shape #750 added coverage for).
  2. Confirm the card draws with Approve / Reject — it does, by design after #750.
  3. Confirm the host's decide() path rejects a status other than pending, so the click cannot resolve the hold.

What changed (expected)

  • The host's decide() uses the same shared judgment the client now uses (isApprovalWaiting, or its host-side equivalent) instead of comparing to the string pending.
  • The statuses the host accepts are derived from that one place, so the client and the host cannot disagree about what Approve may answer.

Acceptance

  • A hold with pending_approval can be approved and rejected through the host path; approved, auto and a missing status are still refused, as today.
  • One test asserts the refused statuses are refused and the waiting ones are not, so a third waiting spelling cannot be added on one side only.
  • Red evidence: put the host comparison back to === 'pending' and the pending_approval case must fail.

Out of scope

Which statuses count as waiting (that lives in approval-waiting.js, from #750), the drawing of the card, and the host's storage format for holds.

Scope widened after triage

The first version of this issue named only the host's decide(). Triaging it against the tree found three more places that read the same status, and they make the defect much wider than one button:

  1. core/src/host/hooks.js:566 — the refusal that starts this issue.
  2. core/src/host/hooks.js:499 (heldOf()) — folds every status that is not sent/pending/failed into rejected, so the chat card still reads "Not sent" for the same hold: isApprovalWaiting('rejected') is false. Fixing the dock while leaving this alone gives two answers on one screen.
  3. core/src/host/hooks.js:413 — treats any non-pending status as finished, so a waiting hold can be cleaned up mid-wait.
  4. core/src/host/hooks.js:397 — PENDING_MAX counts only pending, so waiting holds do not count against the cap.

Where the judgment must live

Move the shared judgment to core/src/shared/approval-waiting.js (the precedent is core/src/shared/mentions.js, already imported by both host and client). The host cannot import from core/src/client/, so leaving it there forces a second copy — and a second copy is exactly what this issue exists to prevent.

Consequences to carry through: rebuild core/client.js and regenerate patches/ds-bot/ (#750 already mirrors that file into the patch directory).

Acceptance (updated)

  • Host and client import the same module; the acceptance is that neither side holds its own literal, not that two copies agree.
  • heldOf() keeps a pending_approval hold as waiting, so the card and the dock say the same thing.
  • The cleanup path (:413) and the cap (:397) count waiting holds as waiting.
  • Red evidence, one per line changed: revert :566 → the approve case fails; revert :499 → the card case fails; revert :413 → the cleanup case fails.

Activity

  1. added
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    area:automationScheduled tasks and automation
    bugSomething isn't working
    on Oct 22, 2026
  2. Birfy commented on Oct 22, 2026

    @Birfy
    CollaboratorAuthor

    Triaged bug + area:automation + agent-ready (Triage, 2026-10-22). Scope written down here so the fix can be small and still complete.

    Premise, checked on main 1bda9b2a (after #750)

    The refusal is one line: core/src/host/hooks.js:566 — if (approval.status !== 'pending') throw … 'is already ${approval.status}'. Retry keeps its own guard at :557-565 (failed only); leave that alone.

    The button really is reachable: the dock asks the host for [...WAITING_STATUSES, 'failed'] (core/src/client/flybot-slots.js:207) and the host's list() filters by whatever is stored (hooks.js:511-521), so a stored pending_approval record is listed and drawn with Approve / Reject while decide() rejects the click. Confirmed as described.

    Honesty about reproduction: nothing on the host writes that spelling today — hold() writes pending (:402-411). The record comes from a plugin-side hold (Bot Social's outbound hold uses pending_approval) or from state written by an earlier build. So the red repro is a unit one: seed state.flybot.approvals[id] with status: 'pending_approval' and call the decide endpoint.

    Two sites the body does not name, same defect

    1. The chat card is still broken after #750. heldOf() folds every status that is not sent/pending/failed into rejected — hooks.js:499 — before the client sees it. isApprovalWaiting('rejected') is false, so the bubble for that same hold renders "Not sent" (held-reply.js:53) while the dock shows Approve. Passing the waiting status through :499 is part of this fix, otherwise one hold is decided by two different answers on one screen.
    2. A waiting hold can be pruned or not counted. :413 treats everything that is not pending as finished and deletes it once the store passes APPROVALS_KEPT; :397 only counts pending toward PENDING_MAX. Both are one-line uses of the same predicate. Without them a waiting hold can vanish (no card, no decision) or let the cap overflow — neither is visible to a UI test.

    invalidateFor (:364) also tests pending; a waiting hold would survive its Bot's deletion as a card that answers "the sender Bot no longer exists" when clicked. Include it if cheap (one line) — and if not, say so in ## Not run / out of scope rather than leaving it unnamed.

    Where the rule lives

    #750 put it in core/src/client/approval-waiting.js, which the host cannot import — so the host would need a second copy, and two copies is exactly what this issue is about. The repository already has the right place: core/src/shared/ (shared/mentions.js is imported by core/src/host/hooks.js and core/src/client/text.js). Move the rule to core/src/shared/approval-waiting.js, import it from the host and from the three client sites, and re-exporting from the old path is not enough for "the client and the host cannot disagree".

    Acceptance (write these before the code)

    • A stored pending_approval hold is approved and rejected through decide(); the record ends approved / rejected, decidedAt set, and approve runs release() exactly as pending does.
    • Refused statuses stay refused with code: 'bot/stale': approved, auto, rejected, stale, denied, failed (without retry), and a record with no status.
    • heldRepliesOf reports the waiting spelling, not rejected; the card asks the shared rule (one test that would pass with a third spelling added on one side only is not enough — assert the client and the host import the same module, e.g. the host-side test imports the rule and the client-side test fails if either site keeps a local literal).
    • Pruning never deletes a waiting status; the PENDING_MAX count includes waiting holds.
    • Red evidence, each revert producing a red test: :566 back to !== 'pending' → pending_approval decide case; :499 back to the fold → the waiting chat-card case; :413 back to !== 'pending' → the prune case.

    Build touch points

    core/client.js must be rebuilt (build-client --check exit 0) and patches/ds-bot regenerated (node scripts/ds-bot-patches.mjs generate, then check exit 0) — #750 mirrored approval-waiting.js into patches/ds-bot/files/src/client/, so a move has to move that file too. Changelog fragment under changelog.d/.

    Out of scope

    Any change to what gets stored when a send is held (the host keeps writing pending), the card's layout, and who may create a pending_approval hold at all.

  3. Birfy commented on Oct 22, 2026

    @Birfy
    CollaboratorAuthor

    Written-by: Dev4

    Claimed. Branch: fix/754-host-waiting-hold (from main); the PR will carry Fixes #754.

    Plan, following the widened acceptance:

    • Move the judgment to core/src/shared/approval-waiting.js and have both host and client import that one module (the shared/mentions.js precedent), so neither side keeps a literal.
    • core/src/host/hooks.js: decide() (:566) accepts a waiting status through the shared predicate; heldOf() (:499) keeps a pending_approval hold waiting instead of folding it into rejected; the cleanup path (:413) and PENDING_MAX (:397) count waiting holds as waiting.
    • Tests: the approve/reject path for pending_approval, the refused statuses (approved, auto, missing) staying refused, heldOf rendering the hold as waiting, cleanup leaving it, and the cap counting it; one assertion that host and client import the same module and hold no literal of their own.
    • Red, one per changed line: revert the decide() predicate → approve case; revert heldOf() → card case; revert the cleanup check → cleanup case.
    • Rebuild core/client.js, regenerate patches/ds-bot/, docs + one changelog fragment.
  4. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 22, 2026
  5. self-assigned this
    on Oct 22, 2026
  6. removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 22, 2026
  7. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 22, 2026
  8. added 2 commits that reference this issue on Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:automationScheduled tasks and automationbugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions