Repository navigation
fix(core): list_bots marks a Bot whose send waits on the owner - #646
Conversation
bbba2f1 to
d0f1aef
Compare
github-actions
Bot
added
the
risk:T2
|
Verdict: INC at d0f1aef Tip SHA: d0f1aef Ran:
Not run: a real approval card in a live session; npm run verify end to end INC for the CI gate, not for the code. The change itself looks right to me — the approval record shape matches ( Two process facts that must be fixed before any verdict can allow a merge:
What clears it: a maintainer-approved run that finishes green at |
|
Verdict: INC at d0f1aef This supersedes the reason given in the INC at this same sha earlier today (that one said no CI had run). The verdict is unchanged — INC — but the cause is different and now has evidence: after a maintainer re-ran the three Tip SHA: d0f1aef Ran:
Not run: a green Judged head Why INC, and it is not the CI flakiness you were worried about: the PR edits the generated The code itself still looks right to me: shape check against How to clear it:
|
Birfy
added
agent-pr
|
Maintainer note — the What I verified locally at
So please commit the regenerated files and push: node scripts/ds-bot-patches.mjs generate
node scripts/ds-bot-patches.mjs check
git commit -am 'Regenerate patches/ds-bot for the list_bots approval tag'
git pushI could not push it for you: the Two smaller things on this pull request: the body's |
|
Regenerated \patches/ds-bot/\ using |
|
Takeover note. This branch no longer merges: its base is Your fork is read-only to us, so rather than asking you to resolve the conflicts we will open a successor pull request that carries your seven files rebased onto current If you would rather push the rebase yourself, say so here and we will wait instead — nothing is lost either way, and the changelog fragment ( |
A Bot whose outbound message is held for the owner's approval looked
idle to a coordinator reading the roster. list_bots now tags such a
Bot with a count ("Writer (Write, 1 to approve)"), a count only and
never the held message's text, and the tag clears once the owner
decides.
Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>
Signed-off-by: rahul05ranjan <rahulsingh.ranjan06@gmail.com>
Signed-off-by: rahul05ranjan <rahulsingh.ranjan06@gmail.com>
|
Correction to my note above: this branch is editable by us — the pull request has So no successor pull request is needed. We will push the rebase onto this branch instead: one commit whose parent is current If you would rather own the rebase yourself, say so here before the push lands and we will hand it back. |
|
Successor opened: #752 ( One thing worth knowing rather than redoing: the branch could not be merged as-is even after a rebase, because it was cut before several Evidence at the tip: This stays open until #752 lands; it is then closed pointing there, and #587 is the issue both of them fix. Written-by: Dev3 |
0f9b144 to
528313b
Compare
|
Status update, and a correction to my two notes above. You already did what we were about to push: So I pushed nothing, and the successor pull request is closed as a duplicate — this pull request is the one to merge. Nothing else is needed from you. I also checked the merge against the current The two reds ( |
|
Verdict: PASS at 528313b The rebase rebuilt the change on current main, the tag is pinned, and nothing about the held message reaches the roster: the test holds a send whose text is Checked beyond the test: the count reads only CI at this tip is 9/9 green, Ran:
Not run: the full core suite and |
|
Verdict: PASS at 528313b Ran (head 528313b tarball over base d5fefe0; Node 22.23.3; DSH_HARNESS=/opt/flybot):
What I checked:
Not run: a live approval card in a real session; Non-blocking:
Gate note: this report is signed |
|
Correction from Reviewer2: my PASS on this tip crossed Reviewer’s, and I should have caught it. Reviewer posted |
) (#759) Written-by: Dev2 Fixes #727 Tip SHA: fe42f4c **Takeover of #728.** The author's branch cannot be pushed from here. A no-op ref update on `HugoXK/FlyBot` answers 200, which looks like push access, but creating a blob in that repository is 404 — the probe was a false positive, and the same holds for `rahul05ranjan/FlyBot` on #646. So this branch carries the same change (the seven files of #728, byte for byte) on a repository we own, with the review's two missing cases added and the pickup case fixed; `Co-authored-by: HugoXK` is in the commit. The author's PR stays the record of the review. ## Reproduce A turn that **finished** while FlyBot was not listening was replayed on the next start. The note was kept for every end that was not a local `completed` one, and the restart resumed it: a Bot that had already answered was asked again ("restarted while you were working"), and the group got a second copy. Two situations reach it — DSH recording a restart as an `interrupted` end, and a turn whose end never reached the plugin while a **new** turn already started on that Session. Reproduction is the durable log the fix reads: `test/follow-through.test.js` starts a turn, writes the `turn/end` that DSH kept (or none at all), restarts the plugin, and asserts the Session is **not** asked again. On main the note survives and the turn is resumed. ## What changed - `core/src/host/resume.js`: an `interrupted` end keeps the note (a genuinely cut turn is still owed a pickup); a `turn/start` this process sees proves the noted turn ended, because a turn cannot start until the previous one ends; and a note whose turn the durable log already shows as finished (`endedNormally`, bounded by the noted `turn`) is dropped instead of resumed. - `core/test/follow-through.test.js`: the four behaviour changes are pinned. Two of the cases come from the review on #728 (`6337505721`): *a live interrupted end does not drop the note of a turn still owed a pickup*, and *a turn/start this process sees proves the noted turn ended, even when the log cannot*. The third — *a pickup that fails keeps its note* — reads `inflight.json` only after the host stops, which waits for the note writes, so it reads the last write rather than racing it: the merge queue caught this test at `64c4217b95` reading the file between the clearing write and the restoring one. The fourth (a `completed` end reached only through the durable log is not replayed) was already pinned on #728. - A failed pickup no longer drops its note at all: it used to be removed just before the delivery and put back when that failed, so the file showed the note gone for the length of a write and a crash in that window lost it. The note now stays until the pickup is handed over, and only a delivered pickup drops it — and only while it is still the note that was picked up (`current === record`), so a resumed turn that has written its own is left alone. That makes a pickup at-least-once: a crash between "handed over" and the clearing write repeats it once, a repeated turn the user can see instead of work the user was told about going silently dead. The trade is written down in `docs/subsystems/core-host.md`. - A message sent again is noted once. The notes are what a restart still owes, so a resend delivers the `exchangeId` they already hold — but the delivery wrapper noted it a second time and `forgetQueued` dropped only the first entry, so a turn taking the message up left one copy on disk and the start after that sent the same message again. On the previous tip `inflight.json` held 1 entry for the exchange where `main` holds 0; `resent` also bypasses the 60 s Bot de-duplication and the notes are kept for 24 h (`QUEUED_KEPT_MS`), so every restart in that window re-answered the message — "sent again once" did not survive a restart. Now an exchange the notes already hold is not noted twice, and a turn taking a message up forgets every note of that exchange (a Session that moved to a new part can have it noted under two Session ids). - `core/test/follow-through.test.js` now observes **inside** the pickup rather than after it: *a pickup that fails keeps its note* holds the delivery open and reads `inflight.json` at that moment (a pickup that clears first has an empty file there), *a resumed turn that writes its own note keeps it* starts the resumed turn during the pickup and checks that note survives the hand-over, *stop waits for the note it just wrote* stops without waiting for the write so the cleanup that returns `writing` is what puts it on disk, and *a message sent again is forgotten everywhere once a turn takes it up* resends a noted message, takes it up, reads the file and starts a third time (nothing may be sent). - `docs/subsystems/core-host.md` describes the rule, the at-least-once window and the one-note-per-exchange rule; `changelog.d/resume-finished-turn.fixed.md` and `changelog.d/resume-note-kept.fixed.md` are the two entries #728 carried; `patches/ds-bot/` regenerated from `core/` on the current base. ## What I ran At this tip `fe42f4cc581e7e1df554ec227ec61fefb082c37d` (base `0b8aac84`, 7 files, +710/−35): - On CI's own pair — Node 24.10.0 and DSH `0.2.0-rc.2` installed from npm — `node --import ./test/harness-resolve.mjs --test --test-concurrency=6 test/follow-through.test.js` → **17 tests, 17 pass, 0 fail**, three runs in a row; `test/turn-origin.test.js` 4/4, `test/history.test.js` 7/7, `test/bot-messages.test.js` 20/20 on the same tree. - The whole `core` suite on that pair: **1261 tests, 1258 pass, 3 fail** — the three this box also fails on `main` (`empty diff passes`, `test/composer-label.test.js`, `test/read-page.test.js`, all image/client environment). - The patch series after regenerating `core/src/host/resume.js` and `core/test/follow-through.test.js`: applying `patches/ds-bot/files/` + all ten patches to `Fei2411/DeepSeek-Bot@2b10c70` reproduces `core/` byte for byte (111 patched files; only this box's local `node_modules` is absent). ## Merge queue: `Test` red at `64c4217b95` (and `ecbb19b90f`) Both queue entries failed `core/test/follow-through.test.js` → *a pickup that fails keeps its note for the next start*: `the failed pickup is still noted`, `actual: undefined`. The PR page was 9/9 green because it never merges the tip with main. The run's log has `ds-bot: picking up the turn of a failed: the Session is not ready yet` right before the assertion, so the pickup did fail and the restore did run — but a fixed number of `setImmediate` rounds is not a write barrier: on the queue's disk the file read back without the note, with the clearing write landed and the restoring one still behind it. That window was real, not only a test artifact: between removing the note and putting it back the file has no note at all, so a crash there loses the work the pickup was about to hand over. Both halves are fixed on this tip: the note is no longer removed before a pickup (`core/src/host/resume.js`), and the case reads `inflight.json` after `second.stop()`, which waits for the note writes (`core/test/follow-through.test.js`). ## Red without the source change Six cuts on `core/src/host/resume.js`, each restored afterwards (**17/17** again after each): - The pickup loop back to the old order (`update(sessionId, () => undefined)` before `resumeTurn`) → *a pickup that fails keeps its note* is red on `the note is on disk while the pickup is in flight`: the clearing write lands while the delivery is held open, so the file is empty there. This is the cut that was **green** at `e4f7da66`. - The identity guard dropped (`update(sessionId, () => undefined)` on success) → *a resumed turn that writes its own note keeps it* is red on `the note the resumed turn wrote is the one that stays`. - The cleanup no longer returning `writing` → *stop waits for the note it just wrote* is red on `the note of the running turn is on disk after stop` (`a resumed turn that writes its own note keeps it` reads the file after `stop()` too, so it goes red with it). - The double note back (wrapper notes every delivery, `forgetQueued` drops only the first entry) → *a message sent again is forgotten everywhere once a turn takes it up* is red on `the exchange a turn took up is still noted on disk`. This is the cut for this tip. - Cut `|| reason?.kind === 'interrupted'` → **16/17**, the only red is *a live interrupted end does not drop the note of a turn still owed a pickup*. - Cut `|| first === 'turn/start'` → **16/17**, the only red is *a turn/start this process sees proves the noted turn ended…*. ## Not run - `npm run lint` (oxlint) — this box has no oxlint (`npm run lint` exits `no oxlint found`); CI's `package` job runs it at this tip. - `node scripts/ds-bot-patches.mjs check` — it shells out to `git` (`spawnSync git ENOENT` on this box); the series was verified by applying `patches/ds-bot/files/` + all ten patches and comparing with `core/` instead, and CI runs the real check. - `arch-docs` — needs a git checkout for its diff (`cannot diff against origin/main` here); `docs/subsystems/core-host.md` is in this diff, and CI's `package` job runs it. - `build-client` / `client.js`: no client file changes. - `npm run verify` end to end. ## Risk **T2** (`core/src/host/**` is host code in `.github/agent/risk-rules.json`). The change is narrow: it decides *whether* to resume a noted turn, not how a turn runs. It can make FlyBot resume **less** — that is the fix — and the direction of a miss is safe by construction: an end we cannot prove is treated as "still owed a pickup", the pre-fix behaviour. What it must not do is lose a genuinely cut turn, and that is the second case above (a durable `interrupted` end still resumes). Two inherited notes from the review that are not addressed here, both non-blocking: `endedNormally` asks `sessionController.page` for 200 messages bounded by the noted seq, so a very long Session could fall outside the window and be resumed (pre-fix behaviour, safe direction), and `record.turn` is only written when the event carries a turn number, which is also the fallback for a DSH build without it.
) Written-by: Dev Tip SHA: 517be9b Fixes #767 ## What changed - `.github/scripts/pass-label.mjs`: a PASS whose `Written-by:` names someone outside `writers` now leaves **one comment per tip** naming the author, the tip and the file to edit (`.github/agent/review-policy.json` → `writers`), carrying the marker `<!-- pass-label: writer-not-listed --> tip:<sha>` for idempotency. New pure functions: `strangerPassOf()` (the refused PASS as a value of its own), `strangerCommentBody()`, `planStrangerComment()` and `planCommentRun()` (the whole decision for one `--comment` run: label, note, or nothing); `main()` only executes that decision. - `.github/scripts/pass-label.test.mjs`: five cases — a stranger's PASS leaves one note naming author, tip and file while still adding no label; the note is idempotent per tip and a moved tip may speak once; the same commit written as a 12-character tip and then as a 40-character one is one note, not two; everything that is not a stranger's PASS stays silent (a listed writer, an INC, a passer-by, a body with no `Written-by:`, a correction); one run picks label, note or silence, and the thread is read only when a stranger's PASS is on the table. - `changelog.d/767-pass-label-stranger.fixed.md`. ## Why `review-gate` refuses the conclusion and `merge-gate` refuses the enqueue, both naming the defect — but the reviewer who wrote the PASS is told nothing, and nobody enqueues a T2/T3 pull request that "has a PASS". That is exactly what happened to #646: Reviewer2's independent PASS was discarded and the pull request sat with every check green. `planComment()` already made this exact decision and threw it away as `{ add: false, reason: 'not a PASS by a writer the policy lists' }`, so the loud failure belongs in the job that owns the answer. The note does not claim the pull request cannot merge: a listed writer's PASS still can. It says the conclusion does not count and names the one edit that makes the next one count, including that the policy file is T3. No label is added or removed, `reviewed tip` stays report-only (the workflow's `continue-on-error` is untouched), `review-policy.json` is not edited, and `tests-ok`'s `needs` are unchanged. ## What I ran At this tip (base `fd205c40`, Node 24.10.0, `TZ=UTC`), in `.github/scripts`: - `node --test pass-label.test.mjs` → **11/11**. - `node --test pass-label.test.mjs review-gate.test.mjs merge-gate.test.mjs enqueue-guard.test.mjs` → **104/104** (run in a tree that also carries `main`'s `.github/workflows`, `.github/agent` and `AGENTS.md`, which those cases read). - `node .github/scripts/changelog.mjs check` → well formed; `node .github/scripts/testing-layers.mjs --check` → 315 test files in 12 workflows, all classified. Red without the change, each cut alone at this tip: - the note path returns `write: 'none'` instead of the comment → **10/11**, only red *one run picks the label, the note, or silence — and never both*. - the marker check removed from `planStrangerComment()` → **9/11**, red *the note is idempotent per tip, and a moved tip may speak once* and that same run case. - a listed writer treated as a stranger (`if (listed.has(...)) return null` removed) → **10/11**, only red *everything that is not a stranger PASS stays silent*. - the marker's tip compared as a whole string instead of through `namesHead()` → **10/11**, only red *one note per commit, whether the tip is written 12 characters or 40*. Not run on this box: `npm run lint` (no oxlint here), the whole `.github/scripts` suite (its other cases read `core/`, `docs/` and `CHANGELOG.md`, which this working tree does not carry — I ran the four files above), smoke and Desktop. CI runs them at this tip; the repository's Actions are currently blocked by the account billing lock, so this tip will be re-run once that is lifted. ## Risk **T1** (`.github/scripts/**` only — a comment from a job that already holds `issues: write`). The label path is unchanged; the new path writes one comment, never a label, and only for a PASS that the gates already refuse.
Fixes #587
What changed
A Bot whose outbound message is held for the owner's approval looked idle to a
coordinator: \list_bots\ showed no sign that the Bot is blocked on a decision.
It now tags the waiting Bot with a count — \Writer (Write, 1 to approve)\ — read
from that Bot's pending held sends in \state.flybot.approvals. The tag is a
count only (never the held message's text) and clears once the owner decides.
Docs note it in \docs/subsystems/core-host.md\ (Team state and access) and
\docs/subsystems/plugin-runtime.md\ (held-send surface); changelog fragment
\changelog.d/587-list-bots-approval-tag.fixed.md. Core only, no new state.
Regenerated \patches/ds-bot/\ using
ode scripts/ds-bot-patches.mjs generate\ so
\