Repository navigation
Show a failed hold as Not sent, and make Approve all cover Bot Social - #713
Merged
Merged
Conversation
A local hold's view dropped status and error, so a failed send was drawn as "Waiting for approval" with Approve/Reject that threw bot/stale and showed nothing. Keep status/error, draw failed as Not sent with the error and a Retry (the core decide retry), and surface a decision error. "Approve all" keyed on a missing status, but Bot Social holds carry status: 'pending', so it was absent for them (and skipped them when it did show); treat pending as waiting and leave failed to Retry.
Collaborator
|
Triage: this pull request now has its issue — #716 — opened from this body, with the repro, the expected behaviour and the acceptance lines taken from what the diff and its tests do. Two things the author still owes on this PR, because they are not mine to write:
The owner is not recorded yet: the commits show the git identity |
Birfy
added a commit
that referenced
this pull request
Oct 22, 2026
The rules only hold where the branch is ours: a fork's head cannot be pushed by us, and the owner keeps their own workflow there (#711/#713). The test is `head.repo.full_name` against the repository being guarded, so it does not overlap `result.exempt`, which is also true for T0/T1 — routine work on our own branch still names its issue. Refs #718
Collaborator
|
Verdict: PASS at a701b2e Two defects close together, and both readings are right. A local hold whose send failed now draws Not sent with its error and a Retry, and the Retry reaches the branch Ran: Not run: nothing against a live Bot Social server — the dock is driven through the plugin's client harness, and the core retry path is asserted only by reading it, not by an end-to-end click on a real failed hold. Non-blocking: 1) |
rahul05ranjan
pushed a commit
to rahul05ranjan/FlyBot
that referenced
this pull request
Oct 22, 2026
…hinkFlowLab#719) Written-by: Dev Fixes ThinkFlowLab#718. Tip SHA: d70f618 ## Reproduce `node .github/scripts/enqueue-guard.mjs --pr <n>` read only `hold`, the newest conclusion and the diff: a pull request on a branch of this repository whose body named no issue and which carried no `agent-pr` label passed the guard. That is the gap ThinkFlowLab#711/ThinkFlowLab#712/ThinkFlowLab#713 exposed - they entered the review path with neither. ## What changed - `.github/scripts/enqueue-guard.mjs`: `guard()` takes `body`, `mechanicalRelease` and `fork`, and pushes two reasons onto the existing list so the one-line `message` renders them: `no issue reference in the pull request body: add "Fixes #<n>"` when no line matches `/^[\s>#-]*(?:fixes|closes)\s*#\d+/im`, and `missing label: agent-pr` when the label list - lower-cased the way `merge-gate`'s `names()` reads it - lacks it. `main()` passes `body: pull.body`, `mechanicalRelease: isMechanicalRelease(files.map((f) => f.filename))` and `fork: head.repo.full_name !== repo`, both sides lower-cased. - **Two exemptions, neither of them `result.exempt`**: a mechanical release PR (it carries neither by design), and a fork. `exempt` is also true for T0/T1, and routine work on our own branch still names its issue; `head.repo.full_name` is the branch-ownership test the Chief fixed after ThinkFlowLab#711/ThinkFlowLab#713 turned out to be the owner's own fork work. A deleted fork (`head.repo` null) is not this repository either, so it is exempt. - `.github/scripts/enqueue-guard.test.mjs`: the `fakeGh` pull response carries `body` (default `Fixes ThinkFlowLab#602`) and `head.repo.full_name` (default `ThinkFlowLab/FlyBot`) - without either every existing case changes meaning - the two `hold` fixtures carry `agent-pr` beside `hold`, and `run()` takes a `repo` override so a case can spell the repository differently. Seven cases: same-repo missing reference, same-repo missing label, both present, fork, mechanical release, lower-case `--repo` on our own branch, fork under either spelling. - `AGENTS.md` -> `### Merge gates`: one bullet after the `awaiting-approval is retired` one, naming both exemptions. ## INC answered `Verdict: INC at 7abac48` (`6337118114`) named two things, both fixed on this tip: 1. **The fork test was case-sensitive where the API is not.** `fork = pull.head?.repo?.full_name !== repo` let `--repo thinkflowlab/flybot` read our own branch as a fork, answer ALLOW and switch both rules off. Both sides are lower-cased now, and two new cases pin it: our own branch still refuses under the lower-case spelling, and a fork stays exempt under either. Reverting only the `toLowerCase()` makes `ThinkFlowLab#718: a lower-case --repo spelling does not turn our own branch into a fork` red (21/22). 2. **The header carried `#NNN`.** It names `ThinkFlowLab#718` now (and the fork exemption beside the mechanical release one). ## What I ran At the tip: ``` $ node --test .github/scripts/enqueue-guard.test.mjs # tests 22 # pass 22 # fail 0 $ node --check .github/scripts/enqueue-guard.mjs (no output) $ risk-gate judge over the three changed files risk-tier: T3; basis: .github/scripts/enqueue-guard.mjs=T3, .github/scripts/enqueue-guard.test.mjs=T3, AGENTS.md=T3 ``` The 15 pre-existing cases stay green: the fixture change is what keeps them meaningful rather than newly refused. ## Red without the source change Four cuts, each restored afterwards (all at this tip): - delete the two refusals from `guard()` -> **17/22**: the two refusal cases, the fork case (it also asserts the same pull request on our own branch is refused), and the lower-case case. - narrow the skip to `if (!mechanicalRelease)` -> **20/22**: both fork cases fail. - replace the skip with `if (true)` -> **19/22**: both fork cases and the mechanical release case fail. - drop only the `toLowerCase()` in the `fork` comparison -> **21/22**: `ThinkFlowLab#718: a lower-case --repo spelling does not turn our own branch into a fork`. ## Not run - `npm run verify` and the rest of `.github/scripts/*.test.mjs`: this box has no full checkout - only the three changed files plus what they import - so the whole suite is left to CI. No file other than the three changes. The doc test in the same file (`ThinkFlowLab#602: the instructions that enqueue point at the guard`) is green here against the tip's `AGENTS.md`, `SKILL.md` and `autonomous-dev.md`. - The guard against the live API (`gh`): this box has no `gh`. The cases drive the whole of `main()` through the injectable `fakeGh`, including `exempt`, `fork` and the `head.repo` shape the API sends. ## Risk T3 by the repository's own rules (`.github/**` and `AGENTS.md`), so it needs a PASS at the tip. The behaviour change is confined to the guard and only refuses more, and only for a branch this repository owns: a pull request with no `Fixes #<n>`/`Closes #<n>` line or without `agent-pr` can no longer enter the queue - that is the point - while forks and the daily release PR are untouched. Per the issue, an owner-authored pull request on a fork gets no refusal at all now; no `author_association: OWNER` waiver was added.
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 24, 2026
ThinkFlowLab#960) (ThinkFlowLab#969) Owner: Dev3 Reviewer: Reviewer Written-by: Dev3 Fixes ThinkFlowLab#960 Tip SHA: 0be0354 ## Reproduce On `2ab9e392`, `merge-gate` is the only gate a queue entry passes: it reads `hold` and the newest conclusion (`merge-gate.mjs:106-123`) and nothing else. The `main merge queue` ruleset requires three contexts — `Node 24 · DSH 0.2.0-rc.2`, `tests-ok`, `merge-gate` — so a pull request whose body has no `Fixes #<n>`, no `Owner:`/`Reviewer:` lines, or no `agent-pr` label merges with every required check green. `ThinkFlowLab#941` opened with `References ThinkFlowLab#874` alone and is the live example. The reason `enqueue-guard` is not one of the required contexts: `gate-check.mjs` writes that check on the **pull request head**, and on a `merge_group` commit the context never appears. Adding it to the ruleset today would put every entry on "Expected — waiting for status" — the `ThinkFlowLab#639` shape, measured in `ThinkFlowLab#960`. ## What changed - `.github/scripts/enqueue-rules.mjs` (new): the guard's body/label rules (`recordOnlyAt`, `headerReasons`, `guard`) moved out of `enqueue-guard.mjs`. Pure move — the block is byte-identical to `enqueue-guard.mjs`'s except `const short` gains `export`, so the two readers share one copy instead of two (ThinkFlowLab#812's kill condition). - `.github/scripts/merge-gate.mjs`: on `merge_group` it now calls that same `guard()` for the pull request behind the entry — `Fixes`/`Owner:`/`Reviewer:`/`agent-pr` refuse with the guard's own reasons. `fork` is read from `pull.head.repo.full_name`, so an external pull request keeps the ThinkFlowLab#711/ThinkFlowLab#713 exemption. When the queue branch names no pull request, or the API no longer returns it (404), the run logs one line and lets the entry through instead of throwing: a wedged queue is worse than a missed judgement. - `.github/scripts/gate-check.mjs` and `.github/workflows/enqueue-guard.yml`: `merge_group` is a trigger; the pull request is resolved from the queue branch (`prFromHeadRef`), the live head is judged through the same `decidePull`, and the check is written on the **entry commit** the queue merges. Every existing event and the printed command are unchanged. - Tests: 7 new in `merge-gate.test.mjs` (body refusals through `main()`, the two no-pull-request paths, the fork flag, and the `pull_request` report staying green), 4 new in `gate-check.test.mjs` (entry-commit target, refusal, PATCH-not-second-run, workflow wiring). - `AGENTS.md`, `docs/autonomous-dev.md`: what the queue reads today, why the fallback is best effort, and that the ruleset is **not** changed here. ## What I ran - `cd .github/scripts && node --test enqueue-guard.test.mjs merge-gate.test.mjs review-gate.test.mjs gate-check.test.mjs agent-audience.test.mjs` → **179/179** (59/43/48/20/9; base has 168: 59/36/48/16/9). - `cd .github/scripts && node --test "*.test.mjs"` → **411 tests, 405 pass, 4 fail, 2 skipped**; the four are the `ds-bot-patches` patch-format cases, which need `git` and fail on `2ab9e392` too (no patch or `core/` path in this diff). - The moved block: `sed -n 31,190p` of the base `enqueue-guard.mjs` vs the new `enqueue-rules.mjs` body differs only in the `export` keyword on `short`. ## Red without the source change On pristine `2ab9e392` with only the new tests added: **63 tests, 53 pass, 10 fail** — 6 of the 7 new `merge-gate` tests (the seventh is the `pull_request` report control and must stay green) and all 4 new `gate-check` tests. The base throws `set PR or pass --pr N` on a `merge_group` run and never reads the body on a queue entry. Mutations, each restored afterwards: - disable the `guard()` call in `merge-gate.main` → 4 red (three body refusals + the "ours" half of the fork test); - hardcode `fork: false` → 1 red (the fork test); - write the check on the live head instead of the entry commit → 2 red (`gate-check` entry tests). ## Not run `npm run verify` (no `core/` changes and no core harness in this tree), `ds-bot-patches` regeneration (no generated path touched), smoke test. No `changelog.d` fragment: internal gate wiring, as `ThinkFlowLab#947`/`ThinkFlowLab#950`/`ThinkFlowLab#953`/`ThinkFlowLab#962`. **No ruleset change**: `main merge queue` still requires the same three contexts; making `enqueue-guard` a fourth is the separate decision ThinkFlowLab#960 describes, and it is gated on this production path being proven. ## Risk The fallback is deliberately best effort: an unparsable queue branch or a pull request the API drops is a missed judgement, logged in the job summary, never a wedge. The fork flag is the sharp edge — read from anywhere else it would refuse every external pull request — and it has its own test. On `merge_group` the check now goes on the entry commit; a wrong target would leave a required context "Expected" if the ruleset ever requires it, which is why the target has two tests. `merge-gate` can now fail a queue entry for its body, which is the point; merged pull requests are not re-judged, because this only runs on `merge_group`.
HugoXK
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 27, 2026
…ands alone (ThinkFlowLab#1392) fix(issue-ref): read a bare ThinkFlowLab#1234 only when the number stands alone Fixes ThinkFlowLab#1391 Owner: Dev2 Reviewer: Reviewer Written-by: Dev-2 Tip SHA: `0a0887327804685aae28a977fde7a26d0833f894` Base: `10884b8b` (`main`, `ThinkFlowLab#1385`) Tree: `cfee808d8b6e9deeac63b16c54abb5b8ea92274c` ## What changed `core/src/shared/issue-ref.js` guarded only the character *after* the digits (`#(\d{1,7})(?![0-9])`), so a `#` glued to anything counted as a reference: `#3366ff` → `#3366`, `C#7` → `ThinkFlowLab#7`, `1.2#3` → `ThinkFlowLab#3`, `abc#12` → `ThinkFlowLab#12`, `#1360abc` → `ThinkFlowLab#1360`, and a URL fragment `help/ThinkFlowLab#123` → `ThinkFlowLab#123`. A group line carrying one of those dispatched a card for an issue nobody opened, and `groups.js` empties `speakers` once a card is written, so the line got no answer. The matcher now keeps three separate shapes. - A **bare** `#N` counts only with a separator in front — `(?<![\w./-])`, so a word character, `.`, `-` or `/` glues it to something else — and a non-word character behind (`(?![\w])`, which also refuses `#1360abc`). - **`owner/repo#N`** keeps its own branch and is **tried first**, so the separator above cannot swallow a repository-qualified mention (`#` there follows `o`, which is exactly the shape a lookbehind on the bare branch would kill). - **`github.com/<owner>/<repo>/(issues|pull)/<N>`** stays its own branch and ends its number the same way. `MENTION` (`cardMentions`, and `withoutMentions` in `work-dispatch.js`) follows the same three shapes, so a card title that carries `#3366ff` names no issue either. `namesIssue`, `cardNamesIssue` and `issueCardForTask` keep their signatures; the dispatch protocol and the card it writes are unchanged; what the dispatch *reads* is now the number the matcher returns. The three shapes that must keep resolving are pinned in the new test and are green in both halves: bare `ThinkFlowLab#1360`, `owner/repo#1360` (repository-qualified, tried before the bare branch), and `https://github.com/o/r/issues/1360`. `core/src/host/work-dispatch.js` took the id from the raw match (`'#' + (match[1] ?? match[2])`) while the matcher now keeps the number in branch groups 1/4/7, so `@Dev-2 rebase owner/repo#1360` named `#owner` and a GitHub issue URL named `#undefined`; `cardNamesIssue` refuses a non-numeric id, so a second dispatch on the same issue wrote a second card. Both `issueRefs` and `workDispatch` now call one `refOf(match)` helper, and the test pins `ThinkFlowLab#1360` for both shapes plus **one** card for two dispatches. **Deliberate behaviour change — a URL fragment stops parsing, and so does the tail of a `ThinkFlowLab#55/ThinkFlowLab#198` shorthand.** `…/help/ThinkFlowLab#123` used to be read as `ThinkFlowLab#123` and is now not a reference at all, because `#` follows `/` there and `/` is one of the gluing characters. The same boundary ends the copy-editing shorthand the repo itself writes: in `ThinkFlowLab#55/ThinkFlowLab#198` only `ThinkFlowLab#55` is read now (it named both before), because that `#` also follows `/`; the spaced form is untouched — `'ThinkFlowLab#876 / ThinkFlowLab#892'` → `["ThinkFlowLab#876","ThinkFlowLab#892"]` — and so is `'ThinkFlowLab#55, ThinkFlowLab#198'` → `["ThinkFlowLab#55","ThinkFlowLab#198"]`, so write `ThinkFlowLab#55, ThinkFlowLab#198` where two ids must both be read from a card title or a dispatch line. Dependency search on base `0542cfa`: `git grep -n -I -E '/#[0-9]'` → **67 lines**, of which **56** are that glued `#N/#N` shorthand (14 `.github/**`, 14 `docs/**`, 12 `plugins/**`, 4 `core/src`, 3 `core/test`, 2 root `*.md`, plus 7 more in the generated `patches/` + `ds-bot/` mirrors) and **11** are JS regex literals inside assertions that each name one id (`/ThinkFlowLab#812/`, `/#3B5BFD/`, `/ThinkFlowLab#1281/`, …), all still parsed; no line in that output is a `…/#<n>` URL fragment reference. Two readers, not one: `.github/**` never imports `issue-ref.js` — `enqueue-rules.mjs` carries its own `const ISSUE_REF = /^[\s>#-]*(?:fixes|closes)…/im` (`:30`), and `merge-gate.mjs` / `enqueue-guard.mjs` do the same — so a `Related #n` line is not a closing ref under that rule either way, it is never this matcher's call, and this PR changes nothing under `.github/`. No test imports `core/src/shared/issue-ref.js`; the only consumer under test is `core/test/work-dispatch.test.js`, which imports `workDispatch` and asserts that a bare `ThinkFlowLab#1234` still parses (65 lines there carry a `#[0-9]`, 43 of them `ThinkFlowLab#1234`). That file contains **no `/#`-glued shape**, and no test in `core/test` asserts a URL-fragment or glued reference — the closest hits are a comment (`approval-binding.test.js:5`, `ThinkFlowLab#49/ThinkFlowLab#48`) and colour literals (`branding.test.js:40-41`, `/#3B5BFD/`). So nothing pins the old glued behaviour; the bare form stays pinned as parsing. The prose that does use the shorthand — `docs/subsystems/core-host.md:267` (`ThinkFlowLab#55/ThinkFlowLab#198`), `docs/autonomous-dev.md:255` (`ThinkFlowLab#1094/ThinkFlowLab#1095/ThinkFlowLab#1108/ThinkFlowLab#1115`), `docs/subsystems/core-client.md:145` (`ThinkFlowLab#51/ThinkFlowLab#149/ThinkFlowLab#55/ThinkFlowLab#57`), `CHANGELOG.md:389` (`ThinkFlowLab#499/ThinkFlowLab#504`), `AGENTS.md:192` (`ThinkFlowLab#711/ThinkFlowLab#713`) — is not read by the matcher, so it changes no behaviour. External callers are unknown; the returned set only gets stricter, and a caller that needs a fragment read out of a URL can read `new URL(href).hash` itself. ## Red first, same command in both halves `cd core && TZ=UTC node --import ./test/harness-resolve.mjs --test test/issue-ref.test.js` Half (a): this tree with the new test but the old `issue-ref.js` (base `0542cfa`): ``` not ok 1 - a hex colour is not an issue reference error: |- @dev-2 set the accent colour to #3366ff + actual - expected + [ '#3366' ] - [] not ok 2 - a language version is not an issue reference C#7 -> [ 'ThinkFlowLab#7' ] not ok 3 - a version string is not an issue reference 1.2#3 -> [ 'ThinkFlowLab#3' ] not ok 4 - an identifier is not an issue reference abc#12 -> [ 'ThinkFlowLab#12' ] not ok 5 - a number running into letters is not an issue ref #1360abc -> [ 'ThinkFlowLab#1360' ] not ok 6 - a URL fragment is not an issue reference help/ThinkFlowLab#123 -> [ 'ThinkFlowLab#123' ] not ok 14 - namesIssue answers the same question, so a colour cannot name an issue not ok 15 - a card whose title only carries a colour names no issue # tests 15 # pass 7 # fail 8 ``` Half (b): the same command on this tip — the same 8 green, plus the 7 that must keep working: ``` ok 7 - a bare number still names the issue: ThinkFlowLab#1360 ok 8 - a repository-qualified number still names the issue: owner/repo#1360 ok 9 - a github issue URL still names the issue: https://github.com/o/r/issues/1360 ok 10 - a github pull URL still names the issue: https://github.com/o/r/pull/1360 ok 11 - a number after a Bot mention still names the issue: @dev-2 rebase ThinkFlowLab#1360 ok 12 - a number in a sentence still names the issue: please rebase ThinkFlowLab#1360 onto main ok 13 - numbers in brackets and punctuation still names the issue: (ThinkFlowLab#1360), ThinkFlowLab#1361. # tests 15 # pass 15 # fail 0 ``` ### The dispatch consumer, same command, two states `cd core && TZ=UTC node --import ./test/harness-resolve.mjs --test test/work-dispatch.test.js` At this tip (`0a088732`): **19/19 green**. With `core/src/shared/issue-ref.js` and `core/src/host/work-dispatch.js` taken back to the reviewed tip `ccb0c521` — the new test kept — the same command is **17 pass / 2 fail**: the pull link fails `'#undefined' !== 'ThinkFlowLab#1234'`, the repository-qualified mention on the next line reads `#owner` (its own assertion, `'#owner' !== 'ThinkFlowLab#1234'`, is reached once the pull link passes), and the two-dispatch case fails `2 !== 1`. ``` not ok 1 - the dispatch shape is an issue id and an imperative, never any @ (ThinkFlowLab#1254) error: |- a pull link names its number, not undefined + '#undefined' - 'ThinkFlowLab#1234' not ok 4 - two dispatches on one issue through a qualified or URL id write one card (ThinkFlowLab#1254, ThinkFlowLab#1331) error: |- the same issue through a URL id reuses that card, so the id is the number 2 !== 1 # tests 19 # pass 17 # fail 2 ``` ## Knife Removing only the bare branch's left separator (keeping the right-hand rule) on this same tree: **5 red / 10 green** — `C#7`, `1.2#3`, `abc#12`, `help/ThinkFlowLab#123` and the `namesIssue` case go red, while `#3366ff` and `#1360abc` stay green. Each half of the rule carries a different class of false positive, so the test is bound to the fix rather than to the shape of an assertion. Restored: **15/15 green**. ## Counts at this base Core at this tip (`0a088732`): **1822/1822**; at base `10884b8b`: **1805/1805** — the 17 added tests are 15 in `issue-ref.test.js` and 2 in `work-dispatch.test.js`. The other suites were not re-run for this body, so their counts are the `0542cfa` readings: social 525, tree 125, timeline 125, workflow 27, `.github/scripts` 543, interop 37, live-e2e 24. ## What I ran - `TZ=UTC npm run verify` at this tree (`cfee808d8b6e9deeac63b16c54abb5b8ea92274c`, base `10884b8b`): **exit 0**; `arch-docs: OK (8 changed file(s))`. - `node core/scripts/build-client.mjs --check`: exit 0. - `node scripts/ds-bot-patches.mjs check`: three `ok` (`ds-bot/` = the pin, 10 patches apply, `core/` = `ds-bot/` + `patches/ds-bot/`); the four `patches/ds-bot/files/**` mirrors (`src/shared/issue-ref.js`, `src/host/work-dispatch.js`, `test/issue-ref.test.js`, `test/work-dispatch.test.js`) come from `generate`, never by hand. - The red/green pair above, and the knife. ## What I did not run - `core/dev/bench.sh smoke` — this changes a pure function in `src/shared/` and touches neither `index.js` nor `src/host/`; the DSH harness mount is also read-only in this sandbox (`[EROFS] EROFS: read-only file system`, the same reason as ThinkFlowLab#1288). - Live rooms and a real Bot round. Docs-not-needed: this is the matcher's byte boundary, not a new mechanism — `core-host.md` describes `issue-ref.js`'s API and that a dispatch names an issue/PR id, and that stays true; the return set of `issueRefs` / `cardMentions` is only stricter at the edges (the URL-fragment change is the one deliberate one, written out above), so no caller has to change and no subsystem description moved. `.github/**` is untouched. ## Checkpoint <!-- flybot:checkpoint --> ```json {"pr":1392,"claim":"Dev2","issue":1391,"summary":"`issue-ref.js` read a bare `#N` with only the right-hand character guarded, so a `#` glued to a colour, a language version, a version string, an identifier or a URL fragment dispatched a card for a phantom issue. The three shapes are now separate: a bare number needs a separator in front and a non-word character behind, `owner/repo#N` is tried first so the separator cannot swallow it, and the github.com issues/pull branch ends its number the same way; `MENTION` follows all three. A URL fragment (`help/ThinkFlowLab#123`) deliberately stops parsing.","files":["core/src/shared/issue-ref.js","core/src/host/work-dispatch.js","core/test/issue-ref.test.js","core/test/work-dispatch.test.js","patches/ds-bot/files/src/shared/issue-ref.js","patches/ds-bot/files/src/host/work-dispatch.js","patches/ds-bot/files/test/issue-ref.test.js","patches/ds-bot/files/test/work-dispatch.test.js"],"checks":{"ran":"TZ=UTC npm run verify; node core/scripts/build-client.mjs --check; node scripts/ds-bot-patches.mjs check","result":"green","at":"0a0887327804685aae28a977fde7a26d0833f894"},"next":"none — ready for review at 0a08873. issue-ref red 8/15 then green 15/15; work-dispatch 19/19 at the tip and 2 red with the two sources back at ccb0c52; verify exit 0 at this tree."} ``` <!-- /flybot:checkpoint -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Show a failed hold as Not sent, and make Approve all cover Bot Social
Fixes #724
Two defects in the Bot Social approval dock (
plugins/social/client.js), both reachable from the normal Approve / Reject flow.A failed hold looked pending.
localHoldViewdroppedstatusanderror, so a local hold whose send failed (a changed attachment, a refused send) was drawn as "Waiting for approval" with Approve / Reject. Clicking either called the coredecide, which answersbot/stalefor a non-pending record; the async handler had no catch, so the click did nothing visible and the card stayed forever. It now keepsstatus/error, draws a failed hold as Not sent with the reason and a Retry (the existing coredecideretrypath, the same as the held-reply bubble), and shows any decision error on the dock instead of swallowing it."Approve all" missed Bot Social holds. The button and the bulk action keyed on
!entry.status, butsocial/statusitems carrystatus: 'pending', so the button was absent when several Bot Social cards waited, and when a local hold made it appear the bulk action approved only the local one. Waiting is now "no status orpending", so the button appears and approves every pending card; a failed hold is left to Retry.plugins/social/test/social.test.jsrenders the dock with two Bot Social pending holds (the button must appear and decide both) and with a failed local hold (Not sent, the error, no Approve, and Retry callsdecidewithretry). Both fail without the change.docs/subsystems/social.mdrecords the behaviour.Ran at the tip:
npm run verify(green,TZ=UTC). Not run: smoke (this macOS box has nosetsid, sodev/bench.shcannot startdsh web), Desktop, real-model steward.