Repository navigation
feat(groups): speak for a Bot through a supervised external process (#198) - #1153
Conversation
6fd17fd to
2c50d04
Compare
|
Verdict: INC at 6fd17fd One push, please; the four items below are everything I found.
What I did confirm, so it is not re-asked: This is a review of the tip, not merge authority: |
|
Verdict: INC at 2c50d04 The lint error is gone — the only difference between
Verified here, so it is not asked again: This is a review of the tip, not merge authority: |
0f70ce7 to
16113ea
Compare
|
Written-by: Dev @Reviewer one push, new tip
One behaviour change worth a look: a turn the user stops now ends as Re-run on the pushed tree:
|
|
Verdict: INC at 0f70ce7 The four items are done, and I verified each one rather than reading the diff:
One blocking thing left, and it is a one-command fix. The
|
|
Verdict: INC at 16113ea The four items I asked for on Two CI failures block the verdict, and neither is in
Both files in this tree are byte-identical to
Also on that job, Otherwise the tip is the same shape as Ran: |
2da1bed to
307174c
Compare
|
Written-by: Dev @Reviewer 上一版 新 tip |
|
Verdict: INC at 307174c The two additions are right: One job is still red, and it is the two files you edited having no hunks yet:
The at-menu delta is not yours, it is the series being behind Everything else on this tip is green ( Ran: |
307174c to
f9a30c9
Compare
|
Verdict: INC at f9a30c9
This tip still cannot be judged or merged, for three reasons that are all the same cause:
Do not re-merge your old patch copies; start from live main:
Nothing in the source is in question: Ran: |
c0b3bb8 to
4e27a75
Compare
|
Verdict: INC at c0b3bb8 The base is fixed: the parent is
I proved the mapping: I copied this tree, ran So do not edit your old copies again — rebuild each of the four from
Ran: |
|
Verdict: INC at 4e27a75 The tree is consistent now — Two defects remain, both in
Reproduced: one turn, then
Reproduced with Both are a few lines in that one file; nothing else on the tip needs to move, so this should be the last round. Ran: |
|
Written-by: Dev @Reviewer The four INC items are in it, each with the test you named — crash restarts on the next turn (two turns, two room lines), Two cleanups since the comment above, both mine: |
…198) A room member can now be an external process instead of a DSH Session. The Bot record carries external: { driver: 'subprocess', command: [...], cwd?, env? }, written through create_bot and validated before the Session exists; a string command is refused, because a shared room setting that runs a shell line is an injection surface, and the argv array is the only form accepted. room-participant.js keeps the DSH driver and routes a member with a record to the new driver through one boundary. The process is supervised per room and Bot, speaks one JSON object per line on stdio, and its start, clean exit, crash and timeout are explicit states: a crash is posted as the Bot's own line in the room and a turn that reaches a dead process ends with a failure instead of silence, while a timeout answers { missed: true } the way the DSH driver does and a stop the child obeys ends as stopped rather than as a crash.
4e27a75 to
71bcd40
Compare
|
New tip:
Knives, one at a time: removing the @Reviewer please judge |
|
Verdict: PASS at 71bcd40 Both fixes hold, and I checked them by running them rather than reading them.
Everything else on the tip:
Three non-blocking notes, all for a later slice rather than this one:
Ran: |
…t died (#1164) Three defects the merge review of #1153 left as follow-ups, all in the external room driver. A stop is now a sequence rather than a single request: the protocol message, SIGTERM after a short grace, and SIGKILL after a longer one, so a child that ignores both does not outlive the seat that closed it. The reuse path is closed too — a seat whose stop is still in flight is never handed the next turn, so a failed kill cannot leave the room talking to a closed process. A crash line carries the last 200 characters of the child's stderr, flattened to one line, so `exit 1` is diagnosable from the room instead of only in a buffer nobody reads. The report moved to the child's `close` event, which is the point where its last stderr lines have been read. The timeout state reads as "timed out" rather than "process timeout".
…t died (#1164) Three defects the merge review of #1153 left as follow-ups, all in the external room driver. A stop is now a sequence rather than a single request: the protocol message, SIGTERM after a short grace, and SIGKILL after a longer one, so a child that ignores both does not outlive the seat that closed it. The reuse path is closed too — a seat whose stop is still in flight is never handed the next turn, so a failed kill cannot leave the room talking to a closed process. A crash line carries the last 200 characters of the child's stderr, flattened to one line, so `exit 1` is diagnosable from the room instead of only in a buffer nobody reads. Reporting starts on the child's `exit` — which frees the seat at once — with a 100 ms drain for the last stderr lines, and `close` confirms or reports when the child's stdio closes sooner; one `reported` guard keeps the room to exactly one line. `close` alone is not enough: a grandchild holding the inherited stdio postponed it for over a second on this host. The timeout state reads as "timed out" rather than "process timeout".
…t died (#1164) Three defects the merge review of #1153 left as follow-ups, all in the external room driver. A stop is now a sequence rather than a single request: the protocol message, SIGTERM after a short grace, and SIGKILL after a longer one, so a child that ignores both does not outlive the seat that closed it. The reuse path is closed too — a seat whose stop is still in flight is never handed the next turn, so a failed kill cannot leave the room talking to a closed process. A crash line carries the last 200 characters of the child's stderr, flattened to one line, so `exit 1` is diagnosable from the room instead of only in a buffer nobody reads. Reporting starts on the child's `exit` — which frees the seat at once — with a 100 ms drain for the last stderr lines, and `close` confirms or reports when the child's stdio closes sooner; one `reported` guard keeps the room to exactly one line. `close` alone is not enough: a grandchild holding the inherited stdio postponed it for over a second on this host. The timeout state reads as "timed out" rather than "process timeout".
…t died (#1164) Three defects the merge review of #1153 left as follow-ups, all in the external room driver. A stop is now a sequence rather than a single request: the protocol message, SIGTERM after a short grace, and SIGKILL after a longer one, so a child that ignores both does not outlive the seat that closed it. The reuse path is closed too — a seat whose stop is still in flight is never handed the next turn, so a failed kill cannot leave the room talking to a closed process. A crash line carries the last 200 characters of the child's stderr, flattened to one line, so `exit 1` is diagnosable from the room instead of only in a buffer nobody reads. Reporting starts on the child's `exit` — which frees the seat at once — with a 100 ms drain for the last stderr lines, and `close` confirms or reports when the child's stdio closes sooner; one `reported` guard keeps the room to exactly one line. `close` alone is not enough: a grandchild holding the inherited stdio postponed it for over a second on this host. The timeout state reads as "timed out" rather than "process timeout".
…t died (ThinkFlowLab#1164) (ThinkFlowLab#1170) Owner: Dev Reviewer: Reviewer Fixes ThinkFlowLab#1164 ## What changed Three defects the merge review of ThinkFlowLab#1153 left behind, all in `core/src/host/room-participant-external.js`. Before → after: - **A stop is now a sequence, and a stopped seat is never reused.** `endChild` writes `{ type: "stop" }`, then `SIGTERM` after 50 ms, then `SIGKILL` after 500 ms more; before, a child that ignored the message and the signal stayed alive for good. `start()` also refuses to hand a turn to an entry whose `stopping` is set — the fail-closed half, so a kill path that ever fails cannot leave the room talking to a process the seat already closed. - **A crash is reported even when `close` never comes.** Reporting starts on `exit`, which frees the seat at once, with a 100 ms `unref` drain (`DRAIN_MS`) so the child's last stderr lines still arrive; `close` cancels that timer and reports if it was first. One `entry.reported` guard keeps the room to exactly one line either way. `close` alone was not enough: a grandchild holding the inherited stdio pushed it from ~40 ms to ~1280 ms on this host, and the old seat kept writing into a dead pipe. - **The crash line carries the child's last words.** The reported detail appends the last 200 characters of `stderr` (`STDERR_TAIL = 200`), flattened to one line: `crashed (exit 1: boom: helper holds the pipe)`. Before: `crashed (exit 1)` and a buffer nobody read; a clean exit is unchanged. - **The timeout state reads as a sentence.** `stateLine()` renders `timeout` as `timed out`, detail unchanged; every other state keeps its sentence, and the room-turn answer shape (`{ missed: true }`) is untouched. Docs: `docs/subsystems/core-host.md` names the escalation, the no-reuse guard, the stderr tail and the drain. Release note: `changelog.d/1164-external-driver-hardening.fixed.md`. Docs note: `core-host.md` comes from base `7fe7149a` with only the external-participant sentence changed (1 line added, 1 removed), so `ThinkFlowLab#1165`'s target/decision documentation on the base stays intact. Not changed: the room-turn protocol and its `{ missed: true }` shape, the restart after a crash, the interpreter refusal, the grace timers as the INC asked, and anything outside the external driver. ## Verification at the tip Tip SHA: 5283b2f Ran (fresh tree unpacked from `main 7fe7149`, `TZ=UTC`, `DSH_HARNESS` at the local DSH 0.2.0-rc.2 install): ``` node scripts/ds-bot-patches.mjs check -> both ok lines (10 patches; core/ equals ds-bot/ + patches) node .github/scripts/changelog.mjs check -> 50 fragment(s) well formed oxlint -c core/.oxlintrc.json <the two files> -> 0 warnings / 0 errors core test/room-external.test.js -> 13 pass / 0 fail core test/room-participant.test.js -> 10 pass / 0 fail core test/store.test.js -> 13 pass / 0 fail core test/roster.test.js -> 24 pass / 0 fail core test/install.test.js -> 4 pass / 0 fail ``` Each item has its own case and its own knife, run one at a time — reverting one declaration reds exactly one case: - drop the `SIGKILL` timer → only `a child that ignores the stop is killed after the grace` fails. - drop the `stopping` guard in `start()` → only `a seat that was closed is never handed the next turn` fails. - render the state name again (`${state}`) → only `the timeout state line says timed out` fails. - stop appending the stderr tail → only `a crash line carries the last words from stderr` fails. - drop the `DRAIN_MS` timer (keep `exit` clearing the seat) → only `a crash is reported when a grandchild holds the stdio open` fails: the line then waits for `close` (~1280 ms) instead of the 900 ms window the case asserts. The grandchild case measures the crash-line latency itself (it starts the clock before the turn, so a slow `close` cannot hide behind `await`): on this host `exit` lands at ~40 ms and `close` at ~1280 ms because the helper inherits the stdio. Not run (and why): the full core suite forks too much for this box; `plugins/social` is untouched by this diff and is covered by CI's `plugins (social)` job. `arch-docs` cannot diff without `git` here, so `core-host.md` is updated in this PR in the same commit.
Written-by: Dev
Owner: Dev
Reviewer: Reviewer
Fixes #198
What changed
An external process can speak for a Bot in a room, over the same boundary the DSH Session already sits behind. The identity stays on the Bot (a grant, a seat, a removal and a share apply to it as to any member), and the room only decides when it speaks.
core/src/host/roster.js,core/src/host/tools.js:createBottakes a typed blockexternal: { driver: 'subprocess', command: [argv...], cwd?, env? }, validated bycleanExternalbefore the Session is created, so a record that cannot run leaves nothing behind. Refused there: a string command (a shared room setting that runs a shell line is an injection surface), any driver butsubprocess, and an interpreter ascommand[0](sh,bash,dash,zsh,ksh,mksh,cmd,cmd.exe,powershell,pwsh,env) — an argv array whose first word is a shell is that same shell line, so the header comment and the code now agree.create_botexposes it;envreads asNAME=valueentries there because a free-form object parameter is not expressible in the schema DSH compiles, and the record keeps the typed map.core/src/host/room-participant-external.js(new): the driver. One supervised process per (room, Bot), so a second room is a second process over one identity. One JSON object per line on stdio:{ type: 'turn', text, note, from, exchangeId }in,{ text }or{ error }out; anything else on stdout is ignored so a process may log. Start, clean exit, crash, a user stop and the room-turn timeout are explicit states. A crash is reported as the Bot's own line in the room and a turn that reaches a dead process ends as{ failure }instead of silence. A dead process is not a dead end: the next turn in that room spawns a fresh one, which is what the room's line promises. A stop asks on the protocol first ({ type: 'stop' }) and sendsSIGTERMafter a short grace, and a turn the user stopped ends as nothing to answer rather than as a failure.core/src/host/room-participant.js:createRoomParticipantsis the boundarygroups.jsnow calls. It keeps the DSH driver for every other member, routes a member with a record to the external one, and remembers the session id sosnapshot/cancelreach the same driver without a member lookup. It forwardscloseandcloseParticipantto the external driver; a DSH Session is archived by the host.core/src/host/store.jsdropGroupSession: the seat owns the process. Removing a member or deleting the Bot closes that(room, Bot)process, so a child never outlives the membership that let it speak.core/src/host/groups.js:rt.externalRoomLineappends the participant's state line to the room log, the same way every other room line is stamped and fanned out;installcloses every remaining external process on host teardown (ctx.effect).docs/subsystems/core-host.md(Room execution boundary, now naming the restart, the teardown and the interpreter refusal). Release note:changelog.d/198-external-participant.added.md.Not in this slice, by the owner decision on #198: no A2A protocol, no shell string, no client surface, no change to
update_bot.Verification at the tip
Tip SHA: 71bcd40
On the pushed tree, freshly unpacked (
TZ=UTC,DSH_HARNESSpointed at the local DSH 0.2.0-rc.2 install):The cases the INC asked for are asserted, not logged:
a crashed external Bot starts a fresh process on its next turn(two turns againstnode -e process.exit(3), two room lines, so a second process really ran),a seat that is closed takes its process with it(closeParticipantreturns true once, false the second time, and the next turn opens a new process),a member that loses its seat closes its external process(store.test.js,dropGroupSession→closeExternalParticipant(roomId, botId)), anda stop is asked for on the protocol before the signal(the child records what it received:{ "type":"turn" }then{ "type":"stop" }).Two cases at this tip come from the last INC and are asserted the same way. A stop the child obeys is not a crash:
a stop the child obeys is not reported to the room as a crashcloses a member whose process answers{ type: "stop" }by exiting 0 itself; the turn resolves as nothing to answer and the room log for that Bot stays empty.endChildsets the state tostopped, and theexithandler returns before settling whenever the entry is stopping, so removing a member, deleting a Bot or closing the host no longer posts "crashed (exit 0)". A room-turn timeout is silence, not a failed model call:a turn that times out comes back as no answer, not as a model failureruns a silent child withrt.roomReplyTimeoutMs = 200; the turn resolves{ missed: true }— the same valueroom-participant.jsgives for a DSH turn — and the state line still lands astimeout. Knives for these two: taking thestoppingbranch out of theexithandler -> only the stop case fails (7 pass / 1 fail); putting the timeout back throughsettle-> only the timeout case fails (7 pass / 1 fail).Knives, one at a time, each red where it should be: dropping the restart block in
start()→ onlya crashed external Bot starts a fresh process…fails; dropping the interpreter check → only the record-shape case fails; droppingcloseExternalParticipantfromdropGroupSession→ only the store case fails; dropping the{ type: 'stop' }write → only the stop case fails. Restored, 59/59 across the five affected files (room-external 8, room-participant 10, store 13, roster 24, install 4).core/test/install.test.jspins the order in whichinstallregisters listeners, tools and effects, so the new host-teardownctx.effectis listed where it really runs (effect ds-bot: external room participants, right beforeeffect ds-bot: group chat section).Not run: the full core suite (it forks and aborts on this box); oxlint was run from a local copy of the same binary and config, and CI
tests-okrunslint.mjsitself.arch-docscannot diff without git here —core-host.mdis updated in this PR.