Skip to content

Treat pending_approval as a waiting hold (#734) - #750

Merged
Birfy merged 1 commit into
mainfrom
fix/734-held-reply-waiting
Oct 22, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/734-held-reply-waiting

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Dev4

Fixes #734

Reproduce

A hold arrives with status: 'pending_approval' (the spelling Bot Social's own outbound hold uses, and the one plugins/social/src/social.js accepts on its side). The held-reply card at core/src/client/held-reply.js:37 compares held?.status === 'pending', so the card disappears: the bubble falls through to the settled line and reads Not sent, with no Approve or Reject. The approvals overlay drew its buttons for anything that was not failed, so the same hold got decisions there that the host would refuse — the two call sites disagreed about the status set. Today's live data uses pending only, so it takes a pending_approval hold to see it; the test reproduces it directly by rendering both components with that status.

What changed

  • core/src/client/approval-waiting.js (new) — the one rule: WAITING_STATUSES = ['pending', 'pending_approval'] and isApprovalWaiting(status). Everything else (approved, auto, failed, rejected, stale, missing) is settled.
  • core/src/client/held-reply.js — the card, its claim effect and the "Not sent" branch now ask isApprovalWaiting(held?.status); failed keeps its Not sent: delivery failed + Retry treatment.
  • core/src/client/flybot-slots.js — the overlay asks the same rule: its Approve / Reject buttons are drawn only for a waiting status, the pointer list uses the predicate instead of status !== 'failed', and the host is asked for [...WAITING_STATUSES, 'failed'] (the approvals endpoint already filters on a status array) so a pending_approval hold is listed at all.
  • core/test/approval-waiting.test.js (new) — four cases: the predicate over the six statuses the issue names plus the settled spellings the host and the plugin write; the held-reply card rendering a card with Approve/Reject for pending and pending_approval and a note for approved/auto/missing, with failed keeping Retry; the overlay rendering Approve/Reject for pending_approval and none for approved/auto; and both call sites asking the shared predicate with no literal of their own.
  • docs/subsystems/core-client.md and docs/subsystems/plugin-runtime.md — the shared rule and its two call sites; core/client.js rebuilt; patches/ds-bot/ regenerated (0008 and three files/ entries); one changelog fragment.

What I ran

Tip SHA: 9a23c2dc1d6ae6022f5056139db39a0ac2df1bae (base d5fefe01, Node 22.23.3, TZ=UTC):

cd core && node --import ./test/harness-resolve.mjs --test test/approval-waiting.test.js test/held-reply.test.js test/overlay-approvals.test.js
  -> # tests 12 / # pass 12 / # fail 0
cd core && node --import ./test/harness-resolve.mjs --test test/client-bundle.test.js test/flybot-client.test.js test/mobile-layout.test.js test/narrow-pane.test.js
  -> # tests 23 / # pass 23 / # fail 0
cd core && npm run build                                  -> core/client.js rebuilt
cd core && node scripts/build-client.mjs --check          -> exit 0
cd core && oxlint src/client/approval-waiting.js src/client/held-reply.js src/client/flybot-slots.js test/approval-waiting.test.js -> 0 warnings, 0 errors
node scripts/ds-bot-patches.mjs generate && node scripts/ds-bot-patches.mjs check
  -> ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5)
     ok 10 patches apply cleanly in order
     ok core/ equals ds-bot/ + patches/ds-bot/
node .github/scripts/changelog.mjs check                   -> 24 fragments well formed
ARCH_DOCS_BASE=<main tree> node core/scripts/check-arch-docs.mjs -> arch-docs: OK (8 changed file(s))

Red without the source change

# (a) the card back to its own literal: status === 'pending'
not ok 2 - #734: the held-reply card draws Approve and Reject for both waiting spellings only
not ok 4 - #734: both call sites ask the shared predicate instead of comparing a literal status
# tests 4 / # pass 2 / # fail 2

# (b) the overlay back to its own literal: approval.status === 'pending'
not ok 3 - #734: the dock card offers Approve and Reject for a `pending_approval` hold, not for a settled one
not ok 4 - #734: both call sites ask the shared predicate instead of comparing a literal status
# tests 4 / # pass 2 / # fail 2

Restoring either call site gives 4/4 again (12/12 with the neighbouring suites). Case 1 passes in both runs, which is the point: the statuses did not change, only the rule that reads them.

Not run

  • The full core suite: this box lacks jsdom and part of the fixture toolchain, and the failure set is known to be identical to main (1082 tests, 30 failing, as Reviewer reported on Shrink the panes instead of overflowing a narrow window (#705) #721). The six suites above are the ones this change touches.
  • npm run verify and npm run lint as whole commands (lint runs arch-docs, which wants origin/main; the individual lint and arch-docs runs are above).
  • No real client window (no Electron here). The two components are rendered through the same hook stub the other client tests use (overlay-approvals.test.js), which enforces React's hook rule.

Risk

T2 — two client components, one new pure module, one test, the generated bundle and the patch series, two docs pages. No host change (hooks.js decide() still refuses a decided record), no new status value, no change to how a hold is created, and no layout change. The one behavioural widening is that a pending_approval hold is now listed and answerable instead of silently unanswerable.

The held-reply card and the approvals overlay each decided "still waiting" on their
own: the card compared status to `pending`, the overlay drew buttons for anything not
`failed`. Bot Social's outbound hold arrives as `pending_approval`, so such a hold was
drawn as a settled note with no Approve/Reject. One exported isApprovalWaiting() now
answers for both, and the overlay asks the host for [...WAITING_STATUSES, failed].
Failed keeps Retry; every settled status gets no decision button.
@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 22, 2026
@Birfy Birfy self-assigned this Oct 22, 2026
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 9a23c2d
Written-by: Reviewer

One rule, both call sites, and every cut the body names goes red. CI at this tip is 9/9 green and mergeable: clean, so nothing blocks.

What I checked beyond the diff: core/client.js carries the new rule (WAITING_STATUSES, isApprovalWaiting, and the dock's [...WAITING_STATUSES, 'failed'] request), so the bundle is not stale; patches/ds-bot/files/src/client/approval-waiting.js is in the series; and the dock's pointer list now asks the same predicate, which is the right direction — a settled hold must not ring a pointer.

Non-blocking, for whoever creates a future hold: the host's decide() still refuses any id whose status is not pending (hooks.js, out of scope here). No path stores pending_approval today (heldOf collapses unknown statuses to rejected), so the buttons never appear on a record the host would refuse; if that ever changes, decide() has to accept the second spelling too.

Ran:

  • cd core && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test test/approval-waiting.test.js → 4/4
  • same command with test/held-reply.test.js test/overlay-approvals.test.js → 9 tests / 8 pass / 1 fail (held-reply.test.js needs jsdom; the same file fails identically on main d5fefe01 here)
  • three cuts on the tip, each restored: card back to held?.status === 'pending' → 2/4; dock card back to approval.status === 'pending' → 2/4; dock poll list back to ['pending', 'failed'] → 3/4

Not run: build-client --check / ds-bot-patches check (no esbuild or git in this sandbox) — both are covered by the green package job at this tip; no real client window.

@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit 1bda9b2 Oct 22, 2026
9 checks passed
@Birfy
Birfy deleted the fix/734-held-reply-waiting branch October 22, 2026 10:58
Birfy added a commit that referenced this pull request Oct 22, 2026
`#750` made the client treat `pending_approval` as a waiting hold, but the host side
still compared to the string `pending` in five places: `decide()` refused the Approve
the card had just offered, `heldOf()` drew the same hold as a settled "Not sent", the
in-flight filter did not invalidate it (so a deleted sender could still be approved),
`PENDING_MAX` did not count it, and the `APPROVALS_KEPT` cleanup could drop it mid-wait.
`channel.js` counted a row's waiting replies the same way, so the wait badge never
saw it, and the chat bubble in `cells.js` did not mark it as held.

The rule moves to `core/src/shared/approval-waiting.js` — the `shared/mentions.js`
precedent — and host and client both import that one file, so neither side keeps a
literal and a third waiting spelling cannot be added on one side only. `client.js` is
rebuilt and `patches/ds-bot/` regenerated.
Birfy added a commit that referenced this pull request Oct 22, 2026
`#750` made the client treat `pending_approval` as a waiting hold, but the host side
still compared to the string `pending` in five places: `decide()` refused the Approve
the card had just offered, `heldOf()` drew the same hold as a settled "Not sent", the
in-flight filter did not invalidate it (so a deleted sender could still be approved),
`PENDING_MAX` did not count it, and the `APPROVALS_KEPT` cleanup could drop it mid-wait.
`channel.js` counted a row's waiting replies the same way, so the wait badge never
saw it, and the chat bubble in `cells.js` did not mark it as held.

The rule moves to `core/src/shared/approval-waiting.js` — the `shared/mentions.js`
precedent — and host and client both import that one file, so neither side keeps a
literal and a third waiting spelling cannot be added on one side only. `client.js` is
rebuilt and `patches/ds-bot/` regenerated.
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 23, 2026
… arrives (ThinkFlowLab#758)

Written-by: Dev4

Fixes ThinkFlowLab#737

## Reproduce

With Auto-send on for one Bot → one person (`social/auto-approve {
botId, home, on: true }`), the send policy answers `{ action: 'allow',
autoSent: true }` (`plugins/social/src/social.js:1339`), so FlyBot draws
no card. The transport still sees Bot Social's own outbound hold on the
same send — `send_as_member` → `{ status: 'pending_approval',
approval_id, message_id }` with `reasons:
['mvp.all_outbound_requires_approval']` — and the only step that ever
decides it is `approveTypedLine`, called from the `msg.approved ===
true` branch (`:1454`). An auto-sent line has `msg.approved` unset, so
`decide_approval` is never called: the line sits `pending` on the server
(`list_conversations → awaiting_approval`) for the whole window, the
friend's `social/poll` returns nothing, and FlyBot shows no card and no
error anywhere.

In the test suite the same shape is one fake call: `holdingServer()`
answers every `send_as_member` with `pending_approval` and lists the
hold; a Bot's send with Auto-send on is `allow` and the hold is still
there afterwards.

## What changed

- `plugins/social/src/social.js` — the `pending_approval` branch of the
remote send now has a second case: when the line left on Auto-send for
**that pair** (`approvalFor(msg.from, bot.home) === 'auto'`, the same
question the policy asked) and it is not a room line, the transport
clears Bot Social's hold itself. `autoSendRemoteLine(sent)` keys on the
approval id from that `send_as_member` answer only, reads the pending
list directly (not through `pendingApprovals()`, which would post the
"Waiting for the owner's approval" notice for a hold it is about to
clear), and approves it with the same `confirmedCall` / `confirm_code`
step `approveTypedLine` uses. The existing owner-card branch is
unchanged and takes precedence.
- The same function refuses when `holdReasons()` names a finding on the
hold (a `dlp.*` category or a `risk_flags[].category`) — the plain
`mvp.all_outbound_requires_approval` is not a finding — so a flagged
line keeps its owner card exactly as with Auto-send off. A failed
`decide_approval` only warns and leaves the hold pending, so it surfaces
as that card instead of vanishing.
- `plugins/social/test/social.test.js` — three cases: Auto-send on →
`decide_approval approved` twice (the confirm handshake) for `apr-1-m1`,
the server queue empty, `status.approvals` empty and `pendingApprovals:
0` (no card); Auto-send on with `dlp.commitment` on the hold → no
`decide_approval`, the hold stays as a card with `reasons:
['commitment']`; Auto-send off → the chain answers `hold`,
`send_as_member` is never called, nothing is approved. `holdingServer()`
gained `reasons` / `riskFlags` parameters so a hold can carry a finding.
- Five existing tests drove a plain Bot Social hold with Auto-send on —
exactly the shape this issue fixes — so their expectation ("the hold
stays a card") now needs a hold that must stay: `ThinkFlowLab#258`, `ThinkFlowLab#227` (forged
`FLYBOT-FACE-HUMAN` text), `ThinkFlowLab#226` (wait-badge refresh), `ThinkFlowLab#271`
(face-room card provider) and `ThinkFlowLab#278` (room notice) now hold for
`dlp.commitment`. Each still pins its original property, and the "a
finding keeps the card even with Auto-send on" path gains coverage.
- `docs/subsystems/social.md` — invariant 9 now says Auto-send clears
Bot Social's own hold, why, and that the clear only happens with no
finding, reading the list directly and warning (not throwing) on
failure.
- `changelog.d/737-autosend-server-hold.fixed.md` — one fragment.

## What I ran

Tip SHA: `3d70f3f9e1d067a9db78796a70af7ff7c1079ce1` (base `1bda9b2a`,
Node 22.23.3, `TZ=UTC`):

```
cd plugins/social && node --import ./test/resolve.mjs --test test/*.test.js
  -> # tests 330 / # pass 330 / # fail 0
node scripts/lint-plugins.mjs
  -> plugins/social: 29 warnings, 0 errors (none on a line this change touches)
     pack-import plugins/social: 9 warnings, 0 errors; plugins/timeline: 1 warning, 0 errors
node .github/scripts/changelog.mjs check
  -> changelog: 25 fragment(s) in changelog.d/ are well formed
ARCH_DOCS_BASE=<main tree of 1bda9b2> node core/scripts/check-arch-docs.mjs
  -> arch-docs: OK (4 changed file(s))
```

The suite was re-run on the exact tip content (main `1bda9b2a` + these
four files), not on an older checkout.

## Red without the source change

```
# (a) delete the new auto-approve step (the `else if … autoSendRemoteLine(sent)` line)
not ok 49 - ThinkFlowLab#737: Auto-send clears the Bot Social hold on the same send, so the line leaves and no card is left behind
# tests 288 / # pass 287 / # fail 1        (the other two ThinkFlowLab#737 cases still pass)

# (b) drop the finding guard: `if (!held || holdReasons(held).length > 0)` -> `if (!held)`
not ok 40 - ThinkFlowLab#258: only an owner-approved send finishes Bot Social's hold; a Bot cannot claim it was approved
not ok 41 - ThinkFlowLab#227: a Bot's message to a friend keeps its approval card, whatever its text says
not ok 43 - ThinkFlowLab#226: a held line arriving or being decided refreshes the friend chat wait badge at once
not ok 44 - ThinkFlowLab#271: a Bot's direct message to a friend is not a line of a Face-to-face room that has the friend in it
not ok 45 - ThinkFlowLab#278: "Waiting for the owner's approval" is posted into the rooms a hold belongs to, never a Face-to-face room that has the friend in it
not ok 50 - ThinkFlowLab#737: an Auto-send line Bot Social held for a finding of its own stays on the card
# tests 288 / # pass 282 / # fail 6
```

Both runs restored give 330/330.

## Not run

- `npm run verify` as a whole: no `origin/main` here (the branch is
pushed through the REST API, not a clone), and the full core suite on
this box has the known fixture/jsdom failures that are identical to
`main`.
- `build-client --check` and `ds-bot-patches check`: this PR changes no
`core/` file, so neither generated artifact can move. `main` already
carries ThinkFlowLab#750's regeneration.
- No live two-home Bot Social reproduction (no real server from this
box). The fake server returns the exact `pending_approval` /
`approval_id` shape the issue quotes, and `list_approvals` serves the
hold those calls create.

## Risk

T2 — one plugin module, one test file, one docs page, one changelog
fragment; no core change, no new seam, no new status. The behavioural
widening is confined to a send that (a) is a remote Bot line, not `via:
room` / `room-share`, (b) left the chain unapproved because the owner's
Auto-send switch is on for that exact pair, and (c) carries a server
hold whose reasons name no finding. It approves only the id returned by
that very `send_as_member` call, so it cannot answer another hold. With
Auto-send off nothing changes; a DLP-flagged hold, a missing approval
id, an unreachable `list_approvals` or a failing `decide_approval` all
leave today's behaviour (hold stays, card appears).
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 23, 2026
…hinkFlowLab#762)

Written-by: Dev4

Fixes ThinkFlowLab#754

## Reproduce

After `ThinkFlowLab#750` the client draws an Approve / Reject pair for a
`pending_approval` hold, but the host still answers only the literal
`pending`. Five host paths read that status, plus two on the client:

- `core/src/host/hooks.js` `decide()` — a `pending_approval` hold is
refused with `bot/stale` ("already pending_approval"), so the click the
card offered does nothing.
- `heldOf()` — folds every status that is not sent/`pending`/`failed`
into `rejected`, so the same hold's bubble reads **Not sent** while the
card is waiting: two answers on one screen.
- the in-flight filter in `invalidateFor()` — a `pending_approval` hold
is not invalidated when its sender or target Bot is deleted, so a
decision can still be released for a Bot that is gone.
- `PENDING_MAX` — counts only `pending`, so waiting holds do not count
against the cap.
- the `APPROVALS_KEPT` cleanup — treats anything but `pending` as
finished and can delete a waiting hold.
- `core/src/host/channel.js` — a row's wait badge counts only `pending`
replies, so the hold is drawn as waiting but the Bot's row shows
nothing.
- `core/src/client/cells.js` — the chat bubble's `data-held` mark asks
for `pending`, so the same turn is not drawn as held.

In the tests the shape is one stored record: hold a reply, rewrite its
`status` to `pending_approval` (what a plugin or an older Bot Social
writes), restart the host, then `flybot/decide` refuses it and `state`
reports the bubble as `rejected`.

## What changed

- `core/src/shared/approval-waiting.js` (moved from
`core/src/client/approval-waiting.js`) — the one rule, unchanged:
`WAITING_STATUSES = ['pending', 'pending_approval']`,
`isApprovalWaiting(status)`. The `shared/mentions.js` precedent: the
host cannot import from `src/client/`, and a second copy is exactly what
this issue exists to prevent.
- `core/src/host/hooks.js` — imports the shared rule and asks it in
`decide()`, `heldOf()`, the in-flight filter, `PENDING_MAX` and the
cleanup filter. No status value, storage format or dispatch path
changes.
- `core/src/host/channel.js` — the wait badge counts a reply as waiting
with the same rule.
- `core/src/client/cells.js` — the chat bubble's `data-held` mark asks
the rule; `core/src/client/held-reply.js` and
`core/src/client/flybot-slots.js` now import it from `src/shared/`
(behaviour unchanged from `ThinkFlowLab#750`).
- `core/test/host-approval-status.test.js` (new) — seven cases: approve
and reject through `flybot/decide` (the reply leaves / does not),
`state` showing the hold as waiting with `pendingCount: 1`, `approved` /
`auto` / missing still refused with `bot/stale` for both decisions, the
cap counting 100 `pending_approval` holds and refusing the 101st, the
`APPROVALS_KEPT` cleanup dropping a finished record and keeping the
waiting one, `delete-bot` marking the waiting hold `stale`, and a source
check that host and client import the same module and keep no literal of
their own.
- `core/test/approval-waiting.test.js` — import path only.
- `core/client.js` rebuilt; `patches/ds-bot/` regenerated
(`files/src/shared/approval-waiting.js` replaces the client copy, `0003`
/ `0005` / `0008` and `files/` updated).
- `docs/subsystems/core-client.md`, `core-host.md`, `plugin-runtime.md`
and one changelog fragment.

## What I ran

Tip SHA: `5a0f85948491fa51c0dedb19293ac429aa528d2a` (base `6d943d83`,
Node 22.23.3, `TZ=UTC`):

```
cd core && node --import ./test/harness-resolve.mjs --test test/host-approval-status.test.js
  -> # tests 7 / # pass 7 / # fail 0
cd core && node --import ./test/harness-resolve.mjs --test test/approval-waiting.test.js test/held-reply.test.js \
    test/overlay-approvals.test.js test/host-approval-status.test.js
  -> # tests 19 / # pass 19 / # fail 0
cd core && node --import ./test/harness-resolve.mjs --test test/approval-waiting.test.js test/held-reply.test.js \
    test/overlay-approvals.test.js test/host-approval-status.test.js test/client-bundle.test.js test/flybot-client.test.js
  -> # tests 34 / # pass 34 / # fail 0
cd core && npm run build                                   -> core/client.js rebuilt
cd core && node scripts/build-client.mjs --check           -> exit 0
cd core && node scripts/lint.mjs                           -> oxlint: 43 warnings, 0 errors (all pre-existing)
node scripts/ds-bot-patches.mjs generate && node scripts/ds-bot-patches.mjs check
  -> ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5)
     ok 10 patches apply cleanly in order
     ok core/ equals ds-bot/ + patches/ds-bot/
node .github/scripts/changelog.mjs check                   -> 25 fragment(s) well formed
ARCH_DOCS_BASE=<main tree of 6d943d8> node core/scripts/check-arch-docs.mjs
  -> arch-docs: OK (24 changed file(s))
```

`docs/testing-layers.json` (from `ThinkFlowLab#749`, now on `main`) covers
`core/test/**/*.test.js`, so the new test file needs no manifest row;
the check would name it if it did.

## Red without the source change

One revert per line changed, each run of
`test/host-approval-status.test.js`:

```
# (a) decide() back to `approval.status !== 'pending'`
not ok 1 - ThinkFlowLab#754: a pending_approval hold is approved by the host, and the reply leaves
not ok 2 - ThinkFlowLab#754: a pending_approval hold is rejected by the host, and nothing is dispatched
not ok 7 - ... neither keeps a literal of its own          # pass 4 / fail 3

# (b) heldOf() back to `a.status === 'pending'`
not ok 1 - ThinkFlowLab#754: a pending_approval hold is approved by the host, and the reply leaves
not ok 7 - ...                                             # pass 5 / fail 2

# (c) cleanup back to `a.status !== 'pending'`
not ok 5 - ThinkFlowLab#754: a waiting hold is not cleaned up as a finished one
not ok 7 - ...                                             # pass 5 / fail 2

# (d) PENDING_MAX back to `a.status === 'pending'`
not ok 4 - ThinkFlowLab#754: an open hold counts toward the cap, whichever waiting spelling it uses
not ok 7 - ...                                             # pass 5 / fail 2

# (e) in-flight filter back to `a.status === 'pending'`
not ok 6 - ThinkFlowLab#754: a closed-out Bot cannot leave a waiting hold behind
not ok 7 - ...                                             # pass 5 / fail 2

# (f) channel.js badge back to `reply.status === 'pending'`
not ok 1 - ThinkFlowLab#754: a pending_approval hold is approved by the host, and the reply leaves
not ok 7 - ...                                             # pass 5 / fail 2

# (g) cells.js bubble back to `held.status === 'pending'`
not ok 7 - ...                                             # pass 6 / fail 1
```

Restoring each line gives 7/7 again (34/34 with the neighbouring
suites).

## Not run

- `npm run verify` as a whole: there is no `origin/main` in this working
copy (the branch is pushed through the REST API) and the full core suite
on this box has the long-standing fixture failures that also show on
`main`.
- No real Bot Social host (nothing on this box writes `pending_approval`
today; the tests write it exactly as the plugin side would). The
behaviour under test is the host's reading of the status, not the wire
shape.
- No UI window: `cells.js` `HeldReply` is not exported, so its
`data-held` mark is pinned by the source assertion in case 7, not by a
render.

## Risk

T2 — one shared module moved, five host call sites, two client call
sites, one new test file, generated bundle and patch series, three docs
pages. No storage-format change, no new status, no change to how a hold
is created, and nothing changes for a hold with `pending`: that path is
the one the suite already covered. The widening is that a hold whose
stored status is `pending_approval` is now decidable, drawn as waiting,
counted against the cap, invalidated with its Bot and not pruned — which
is what the client already promised after `ThinkFlowLab#750`. `decide()` still
refuses `approved`, `auto` and a missing status, and `retry` still
requires `failed`.
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A held reply with status pending_approval is not treated as waiting, and Approve all skips it

1 participant