From ba4f8ce1829ee4da7e30e6e62507e3106745e2bb Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Mon, 9 Nov 2026 01:24:34 +0800 Subject: [PATCH] Keep this host's paths and keys out of room lines A room line is not local: fanoutRoomLine sends it to every other home in the room, so an unread picture must not name an absolute path of this machine and a crashed external Bot's line must not carry the child's stderr tail. The group placeholder now names the attachment by file name, media type and size only, the crash line reports just exit or signal and keeps the tail in rt.warn, and the store gate every writer already passes through runs this host's secret redaction before a line is kept or sent. Fixes #2699 --- changelog.d/2699-room-line-leak.fixed.md | 1 + core/src/host/groups.js | 5 +- core/src/host/images.js | 8 +- core/src/host/room-context.js | 13 +- core/src/host/room-membership.js | 2 +- core/src/host/room-participant-external.js | 27 +- core/test/images.test.js | 3 + core/test/room-context.test.js | 19 ++ core/test/room-external.test.js | 28 +- core/test/room-line-leak.test.js | 273 ++++++++++++++++++ core/test/session-identity.test.js | 14 +- docs/subsystems/core-host.md | 7 +- patches/ds-bot/0001-core-fixes.patch | 31 +- patches/ds-bot/0012-room-membership.patch | 7 +- patches/ds-bot/files/src/host/room-context.js | 13 +- .../ds-bot/files/src/host/room-membership.js | 2 +- .../src/host/room-participant-external.js | 27 +- .../ds-bot/files/test/room-context.test.js | 19 ++ .../ds-bot/files/test/room-external.test.js | 28 +- .../ds-bot/files/test/room-line-leak.test.js | 273 ++++++++++++++++++ 20 files changed, 729 insertions(+), 71 deletions(-) create mode 100644 changelog.d/2699-room-line-leak.fixed.md create mode 100644 core/test/room-line-leak.test.js create mode 100644 patches/ds-bot/files/test/room-line-leak.test.js diff --git a/changelog.d/2699-room-line-leak.fixed.md b/changelog.d/2699-room-line-leak.fixed.md new file mode 100644 index 000000000..374b106a0 --- /dev/null +++ b/changelog.d/2699-room-line-leak.fixed.md @@ -0,0 +1 @@ +A room line no longer tells the other homes in the room about this machine: a picture that could not be read is named by its file name and size instead of its absolute path (a Bot's own chat still names the path for itself), and a crashed external Bot's line reports only how it ended (`exit 1`, `signal SIGKILL`) while the child's last `stderr` words stay in this host's log. Every stored room line also passes this host's secret redaction before it is kept or sent, so a pasted key cannot ride a line to a friend's Bot ([#2699](https://github.com/ThinkFlowLab/FlyBot/issues/2699)). diff --git a/core/src/host/groups.js b/core/src/host/groups.js index f31897ee7..da68f2504 100644 --- a/core/src/host/groups.js +++ b/core/src/host/groups.js @@ -30,7 +30,7 @@ import { FACE_LEAD_NOTE, FACE_LEAD_ROUTE, faceLeadIsRemote, faceMemberNote, face import { faceMentions } from './face-mentions.js' import { cleanName, stripSessionChips, unwrapUntrusted } from './delivery.js' import { createRoomParticipants } from './room-participant.js' -import { ROOM_CONTEXT_BUDGET, boardCoversCue, fitRoomLines, formatRoomBoard, formatRoomPresence, prepareRoomLine } from './room-context.js' +import { ROOM_CONTEXT_BUDGET, boardCoversCue, fitRoomLines, formatRoomBoard, formatRoomPresence, prepareRoomLine, setRoomLineRedactor } from './room-context.js' import { workDispatch } from './work-dispatch.js' import { retiredBot } from './store.js' @@ -236,6 +236,9 @@ export function install(rt) { const participant = createRoomParticipants(rt) fanoutRoomLine = (room, line) => rt.fanoutRoomLine?.(room, line) stampRoomLine = (room, line) => rt.stampHop?.(room, line) ?? line + // A stored room line leaves for the room's other homes, so the store gate redacts with this host's + // own redactor (#2699). + setRoomLineRedactor(text => rt.redactSecrets?.(text) ?? text) rt.notePostedLine = (roomId, text) => postedLines.set(roomId, [...(postedLines.get(roomId) ?? []), text].slice(-20)) // What an external participant's process does is a room line (#198), so a crash is seen where // the Bot speaks and not only in a log nobody reads. diff --git a/core/src/host/images.js b/core/src/host/images.js index 60ba3c4e6..d1e642913 100644 --- a/core/src/host/images.js +++ b/core/src/host/images.js @@ -72,9 +72,11 @@ const imageFacts = attachment => { } // One placeholder for both places an unread image becomes text: a Bot chat (`imageAsText`) and the -// group relay (`imagesForGroup`). It names the bytes only when `path` is a file this Bot can open. +// group relay (`imagesForGroup`). It names the bytes only when `path` is a file this Bot can open, and +// never in the group form: a group line is fanned out to the room's other homes (#2699), where a path +// of this host says nothing the far side can open and names this machine's folders to it. export function imagePlaceholder(attachment, { path, group = false } = {}) { - const where = path === undefined ? '' : ` The file is at "${path}"; your own file tools can open it.` + const where = group || path === undefined ? '' : ` The file is at "${path}"; your own file tools can open it.` const cannot = group ? '' : 'you cannot see images and ' return `[Image ${imageLabel(attachment)}${imageFacts(attachment)}: ${cannot}it could not be read right now.${where} Ask the user what it shows if it matters.]` } @@ -207,7 +209,7 @@ export function install(rt) { const reading = await imageReading(attachment, sessionId) const label = imageLabel(attachment) return reading === null - ? imagePlaceholder(attachment, { path: await attachmentFilePath(attachment, attachmentServices()), group: true }) + ? imagePlaceholder(attachment, { group: true }) : `[Image ${label}, read by ${reading.model}:]\n${reading.text}\n[End of image ${label}]` })) } diff --git a/core/src/host/room-context.js b/core/src/host/room-context.js index f37fc5f6f..cbd891405 100644 --- a/core/src/host/room-context.js +++ b/core/src/host/room-context.js @@ -105,9 +105,20 @@ const FENCED_LINE = /^\n?([\s\S]*?)\n?<\/untru // Bot speech only. A person's line, and a short line, stay as stored. The log // object is not modified: a collapsed line is a copy. -export function prepareRoomLine(line) { +// +// Every writer of `room.log` passes through here — `groups.js` `appendRoom` (a group round) and +// `room-membership.js` `append` (a person's line, a seat's `line` op, a face-room event) — and the log +// is fanned out to the room's other homes (`fanoutRoomLine`), so the redaction this host trusts runs +// once, on this gate, instead of in each builder of a line (#2699). `redact` is that host's +// `rt.redactSecrets`, wired once at install; a line the redactor leaves alone comes back unchanged. +let redactRoomLine = text => text +export function setRoomLineRedactor(redact) { redactRoomLine = redact } + +export function prepareRoomLine(line, { redact = redactRoomLine } = {}) { if (line == null || typeof line !== 'object') return line if (typeof line.text !== 'string') return line + const clean = typeof redact === 'function' ? redact(line.text) : line.text + if (clean !== line.text) line = { ...line, text: clean } // A line a writer already fenced for another home's Bot keeps its fence, and while it is fenced // neither `kind` nor `authorKind` is an exemption: both travel from the other home // (room-membership.js copies them), so a peer could otherwise mark an essay `notice` or type it as a diff --git a/core/src/host/room-membership.js b/core/src/host/room-membership.js index 54543b51f..82d3764be 100644 --- a/core/src/host/room-membership.js +++ b/core/src/host/room-membership.js @@ -413,7 +413,7 @@ export function install(rt) { // one (postRoomMessage, a seat's `line` op, a face-room event), so the same rule runs here. The rule // itself knows a fence now (`room-context.js` `prepareRoomLine`): it collapses the inside and wraps it // again, so a guest's essay is never stored as this home's own text, and both writers share one copy. - const storeLine = (line) => prepareRoomLine(line) + const storeLine = (line) => prepareRoomLine(line, { redact: rt.redactSecrets }) const append = (room, line) => { ensureLog(room) diff --git a/core/src/host/room-participant-external.js b/core/src/host/room-participant-external.js index 5a83c87a4..9f852ad2d 100644 --- a/core/src/host/room-participant-external.js +++ b/core/src/host/room-participant-external.js @@ -81,12 +81,14 @@ export function createExternalRoomParticipant(rt) { return `${member.name}'s external process ${state === 'timeout' ? 'timed out' : state}${detail ? ` (${detail})` : ''}.` } - // A crash line carries the child's last words, flattened and capped, so `exit 1` is diagnosable - // from the room itself (#1164). + // The child's last words, flattened and capped, are this host's diagnosis (#1164). const tailOf = (entry) => String(entry.stderr ?? '').slice(-STDERR_TAIL).replace(/[\r\n]+/g, ' ').trim().slice(0, STDERR_TAIL) // The room sees what happened to its participant, not just a turn that never came back (#198). - async function report(room, member, state, detail) { - rt.warn?.('ds-bot: external Bot %s in %s: %s%s', member.name, room.name, state, detail ? ` (${detail})` : '') + // `roomDetail` is the part the room may read — an exit code, a signal name, a timeout reason — and + // `logDetail` is this host's diagnosis (the stderr tail, a spawn error naming a file): a room line is + // fanned out to the room's other homes, so only the log keeps the detail (#2699). + async function report(room, member, state, roomDetail, logDetail = roomDetail) { + rt.warn?.('ds-bot: external Bot %s in %s: %s%s', member.name, room.name, state, logDetail ? ` (${logDetail})` : '') try { await rt.externalRoomLine?.(room, { who: member.name, @@ -94,7 +96,7 @@ export function createExternalRoomParticipant(rt) { authorId: member.id, authorKind: 'bot', kind: 'external', - text: stateLine(member, state, detail), + text: stateLine(member, state, roomDetail), }) } catch (error) { rt.warn?.('ds-bot: the state of %s in %s was not posted: %s', member.name, room.name, error) @@ -148,7 +150,7 @@ export function createExternalRoomParticipant(rt) { }) } catch (error) { settle(entry, 'crashed', oneLine(error?.message ?? error)) - void report(room, member, 'crashed', entry.detail) + void report(room, member, 'crashed', undefined, entry.detail) return entry } entry.child = child @@ -192,7 +194,7 @@ export function createExternalRoomParticipant(rt) { entry.child = undefined entry.reported = true settle(entry, 'crashed', oneLine(error?.message ?? error)) - void report(room, member, 'crashed', entry.detail) + void report(room, member, 'crashed', undefined, entry.detail) }) const reportExit = (code, signal) => { // One line either way: whichever of 'exit' and 'close' reports first wins (#1164). @@ -202,11 +204,12 @@ export function createExternalRoomParticipant(rt) { // room should hear about: it stopped when asked, so no 'crashed' line (#198). if (entry.state === 'stopped' || entry.stopping === true) return const clean = code === 0 - const detail = signal !== null ? `signal ${signal}` : `exit ${code}` + const ending = signal !== null ? `signal ${signal}` : `exit ${code}` const tail = clean ? '' : tailOf(entry) - const said = tail === '' ? detail : `${detail}: ${tail}` - settle(entry, clean ? 'exited' : 'crashed', said) - void report(room, member, entry.state, said) + // The room hears how the process ended; its last words stay in the log, where the owner reads + // them and a seat on another home cannot (#2699). + settle(entry, clean ? 'exited' : 'crashed', tail === '' ? ending : `${ending}: ${tail}`) + void report(room, member, entry.state, ending, entry.detail) entry.stopping = false } // 'exit' must free the seat at once — otherwise the next turn writes into a dead child and the @@ -339,7 +342,7 @@ export function createExternalRoomParticipant(rt) { entry.child.stdin.write(`${JSON.stringify({ type: 'turn', text, note, ...(by ? { from: by } : {}), exchangeId })}\n`) } catch (error) { settle(entry, 'crashed', oneLine(error?.message ?? error)) - void report(room, member, 'crashed', entry.detail) + void report(room, member, 'crashed', undefined, entry.detail) } return done } diff --git a/core/test/images.test.js b/core/test/images.test.js index 3d4194b95..767d80aa6 100644 --- a/core/test/images.test.js +++ b/core/test/images.test.js @@ -112,6 +112,9 @@ test('#779: the placeholder names the file, its size and its type; without a pat const group = imagePlaceholder(attachment, { path: '/p/x', group: true }) assert.match(group, /\[Image "IMG_2431\.jpeg" \(image\/jpeg, 1080x1596, 213569 bytes\): it could not be read right now\./) assert.doesNotMatch(group, /you cannot see images/) + // #2699: the group line is fanned out to the room's other homes, so it never names a path of this + // host — not even when one is passed in. + assert.doesNotMatch(group, /The file is at/, 'a group line is read on the far side: no path of this host') // An unnamed image keeps the sha prefix the host has always shown. assert.match(imagePlaceholder({ attachmentId: id }), /\[Image sha256:bdc77c70:/) diff --git a/core/test/room-context.test.js b/core/test/room-context.test.js index c850c851c..f6b226c62 100644 --- a/core/test/room-context.test.js +++ b/core/test/room-context.test.js @@ -36,6 +36,25 @@ test('prepare collapses bot status and leaves the stored line and a person line' assert.equal(prepareRoomLine(short), short) }) +test('the store gate runs the host redactor on every line, and leaves a clean one alone (#2699)', () => { + // A room line leaves for the room's other homes (`fanoutRoomLine`), so the one gate both writers of + // `room.log` pass through is where the host's redactor runs (`rt.redactSecrets`, a stub here). + const redact = text => text.replace(/ghp_[A-Za-z0-9]+/g, '[secret:GITHUB_TOKEN]') + const person = { seq: 1, who: 'User', text: 'the key is ghp_FAKE2699, keep it here' } + const stored = prepareRoomLine(person, { redact }) + assert.match(stored.text, /\[secret:GITHUB_TOKEN\]/) + assert.equal(person.text.includes('ghp_FAKE2699'), true, 'the log line is not rewritten') + const bot = { seq: 2, who: 'Dev', botId: 'dev', text: 'ghp_FAKE2699 is what I pasted' } + assert.match(prepareRoomLine(bot, { redact }).text, /\[secret:GITHUB_TOKEN\]/) + // A bot essay is collapsed after the redaction, so a secret in it is gone whichever end it was on. + const essayBot = { seq: 3, who: 'Dev', botId: 'dev', text: essay('ghp_FAKE2699') } + assert.doesNotMatch(prepareRoomLine(essayBot, { redact }).text, /ghp_FAKE/) + const clean = { seq: 4, who: 'User', text: 'nothing to redact here' } + assert.equal(prepareRoomLine(clean, { redact }), clean, 'a line with no secret keeps its identity') + // Without the host's redactor (a caller that has none) the line is prepared as before. + assert.equal(prepareRoomLine(person), person) +}) + test('the transcript stays inside the budget and prefers a newer short line over an older essay', () => { const lines = [] for (let seq = 1; seq <= 8; seq += 1) lines.push({ seq, who: 'Dev', botId: 'dev', text: essay(`marker-${seq}`) }) diff --git a/core/test/room-external.test.js b/core/test/room-external.test.js index 4bd203597..8ea0d5d79 100644 --- a/core/test/room-external.test.js +++ b/core/test/room-external.test.js @@ -11,10 +11,11 @@ const DIE = 'process.exit(3)' function fixture(t) { const lines = [] + const warnings = [] const rt = { live: new Map(), groupOwners: new Map(), roomWaiters: new Map(), workDirOf: () => undefined, - warn: () => {}, + warn: (...args) => warnings.push(args.join(' ')), externalRoomLine: async (room, line) => { lines.push({ roomId: room.id, ...line }); return line }, } const driver = createExternalRoomParticipant(rt) @@ -25,7 +26,7 @@ function fixture(t) { room, member, sessionId: `external:ops:${member.id}`, text, note: 'your turn', signal: controller.signal, started: [], by: 'chief', exchangeId: `x-${member.id}`, }) - return { driver, rt, room, lines, run } + return { driver, rt, room, lines, warnings, run } } test('an external Bot whose process dies answers with a failure and lands in the room, not in silence (#198)', async (t) => { @@ -233,20 +234,27 @@ test('the timeout state line says timed out (#1164)', async (t) => { assert.doesNotMatch(entry.text, /process timeout/) }) -test('a crash line carries the last words from stderr (#1164)', async (t) => { +test('a crash line reports the ending, and the last words stay in this host\'s log (#1164, #2699)', async (t) => { const BOOM = "process.stderr.write('boom: missing config\\n',()=>process.exit(3))" - const { lines, run } = fixture(t) + const { lines, warnings, run } = fixture(t) const member = { id: 'boom', name: 'Boom', external: { driver: 'subprocess', command: [process.execPath, '-e', BOOM] } } assert.equal((await run(member))?.failure?.reason, 'crashed') const entry = await waitForLine(lines, 'boom') - assert.match(entry.text, /crashed \(exit 3: boom: missing config\)/) + assert.match(entry.text, /crashed \(exit 3\)/) + // The room reads how the process ended; its last words name this host's files, and the room's line + // is read by the other homes in it (#2699). + assert.doesNotMatch(entry.text, /missing config/) + assert.ok(warnings.some(line => line.includes('boom: missing config')), 'the log keeps what the child said') // The tail is capped: 400 characters of stderr keep only the last 200. const NOISE = "process.stderr.write('x'.repeat(400),()=>process.exit(3))" const noisy = { id: 'noise', name: 'Noise', external: { driver: 'subprocess', command: [process.execPath, '-e', NOISE] } } assert.equal((await run(noisy))?.failure?.reason, 'crashed') const noise = await waitForLine(lines, 'noise') - assert.match(noise.text, /x{200}/) - assert.doesNotMatch(noise.text, /x{201}/) + assert.match(noise.text, /crashed \(exit 3\)/) + assert.doesNotMatch(noise.text, /x{2,}/) + const logged = warnings.find(line => line.includes('xxxxxxxx')) ?? '' + assert.match(logged, /x{200}/) + assert.doesNotMatch(logged, /x{201}/) }) test('a crash is reported when a grandchild holds the stdio open (#1164)', async (t) => { // On the first launch the child leaves a grandchild on its inherited stdio and exits 1, so @@ -256,14 +264,16 @@ test('a crash is reported when a grandchild holds the stdio open (#1164)', async const HELD = "const fs=require('node:fs');const cp=require('node:child_process');if(!fs.existsSync(process.env.MARK)){fs.writeFileSync(process.env.MARK,'1');fs.writeSync(2,'boom: helper holds the pipe\\n');cp.spawn(process.execPath,['-e','setTimeout(()=>{},1200)'],{stdio:'inherit'});process.exit(1)}let b='';process.stdin.setEncoding('utf8');process.stdin.on('data',c=>{b+=c;let i;while((i=b.indexOf('\\n'))!==-1){const l=b.slice(0,i);b=b.slice(i+1);const m=JSON.parse(l);if(m.type==='turn')process.stdout.write(JSON.stringify({text:'again: '+m.text})+'\\n')}})" const dir = mkdtempSync(join(tmpdir(), 'flybot-external-')) const mark = join(dir, 'mark') - const { lines, run } = fixture(t) + const { lines, warnings, run } = fixture(t) const member = { id: 'held', name: 'Held', external: { driver: 'subprocess', command: [process.execPath, '-e', HELD], env: { MARK: mark } } } const started = Date.now() const first = run(member) // 900 ms is before the grandchild's 1.2 s: only reporting from 'exit' can meet this. const entry = await waitForLine(lines, 'held', 900) const elapsed = Date.now() - started - assert.match(entry.text, /crashed \(exit 1: boom: helper holds the pipe\)/) + assert.match(entry.text, /crashed \(exit 1\)/) + assert.doesNotMatch(entry.text, /helper holds the pipe/) + assert.ok(warnings.some(line => line.includes('helper holds the pipe')), 'the log keeps what the child said') assert.ok(elapsed < 900, `the crash line waited for close (${elapsed} ms)`) assert.equal((await first)?.failure?.reason, 'crashed') // The seat was cleared on 'exit', so the next turn reaches a fresh process, not a dead pipe. diff --git a/core/test/room-line-leak.test.js b/core/test/room-line-leak.test.js new file mode 100644 index 000000000..1b69e3ca2 --- /dev/null +++ b/core/test/room-line-leak.test.js @@ -0,0 +1,273 @@ +// #2699: a room line is not local. `spawnShare` fans every stored line to the room's remote seats +// (`room-membership.js` `fanoutRoomLine`), so whatever this host writes into a line is read on the +// other side. Two builders used to put this host's own detail there: the unread-image placeholder +// named the attachment's absolute path, and a crashed external process's line carried the last 200 +// bytes of the child's stderr (a child that dies at startup names the file it could not open, and +// may have printed a token). What is asserted below is the decoded room-share envelope the far side +// receives — never the local log, which may keep the detail for the owner. +import assert from 'node:assert/strict' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { install as installStore } from '../src/host/store.js' +import { install as installMemberAuth } from '../src/host/member-auth.js' +import { install as installAuthz } from '../src/host/authz.js' +import { install as installRooms } from '../src/host/room-membership.js' +import { install as installReach } from '../src/host/reach.js' +import { install as installRoster } from '../src/host/roster.js' +import { install as installGroups } from '../src/host/groups.js' +import { attachmentObjectPath, imagePlaceholder, install as installImages } from '../src/host/images.js' +import { createExternalRoomParticipant } from '../src/host/room-participant-external.js' +import { parseRoomShare } from '../src/shared/room-share.js' + +// What the child could not open — it only has to look like this host's layout. +const HOST_PATH = '/data/dsh/bot/home/state.json' +const TOKEN = 'ghp_FAKE2699FAKE2699FAKE2699FAKE2699' + +const TEAM = { + version: 2, + revision: 1, + mainBotIds: ['chief-a'], + members: { 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'chief-a' } }, + bots: { + 'chief-a': { id: 'chief-a', sessionId: 'chief-a', name: 'Chief A', instructions: '', createdAt: 1, ownerId: 'owner-1' }, + 'chief-b': { id: 'chief-b', sessionId: 'chief-b', name: 'Chief B', instructions: '', createdAt: 2, ownerId: 'owner-1' }, + 'spec-a': { id: 'spec-a', sessionId: 'spec-a', name: 'Writer A', instructions: '', createdAt: 3, ownerId: 'owner-1' }, + 'spec-b': { id: 'spec-b', sessionId: 'spec-b', name: 'Writer B', instructions: '', createdAt: 4, ownerId: 'owner-1' }, + }, + rooms: { + war: { + id: 'war', + name: 'War room', + members: ['chief-a', 'chief-b'], + admin: 'chief-a', + named: true, + log: [], + seq: 0, + seen: {}, + participants: [ + { kind: 'human', id: 'owner-1', role: 'admin', displayName: 'Alice', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'chief-a', role: 'admin', displayName: 'Chief A', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'chief-b', role: 'member', displayName: 'Chief B', joinedSeq: 0, joinedAt: 1 }, + ], + }, + }, + exchanges: [], + exchangeTurns: {}, + questions: {}, +} + +const CODER = { handle: '@coder.bob.home', name: 'Coder', home: '@bob.home', homeName: 'Bob', owner: 'Bob', plugin: 'social', remote: true } + +function host(t, saved = TEAM) { + const dir = mkdtempSync(join(tmpdir(), 'ds-bot-room-line-')) + writeFileSync(join(dir, 'state.json'), typeof saved === 'string' ? saved : JSON.stringify(saved)) + const warnings = [] + const disposers = [] + const ctx = { + logger: { warn: (...args) => warnings.push(args.join(' ')) }, + effect(execute) { + const dispose = execute() + if (typeof dispose === 'function') disposers.push(dispose) + }, + on: () => () => {}, + tools: { register: () => {} }, + systemPrompt: { section: () => () => {} }, + sessions: {}, + get: () => undefined, + set: () => {}, + } + const sent = [] + const rt = { + ctx, + home: dir, + statePath: join(dir, 'state.json'), + autoRoomName: () => 'Room', + retitleRoom: async () => {}, + dropGroupSession: () => {}, + warn: (...args) => warnings.push(args.join(' ')), + flybotSend: async (msg) => { sent.push(msg); return { action: 'sent' } }, + } + installStore(rt) + installMemberAuth(rt) + installAuthz(rt) + installRooms(rt) + installReach(rt) + installRoster(rt) + installGroups(rt) + t.after(async () => { + await Promise.all(disposers.splice(0).map(cleanup => cleanup())) + rmSync(dir, { recursive: true, force: true, maxRetries: 5 }) + }) + return { rt, sent, dir, warnings } +} + +const publish = (rt, list = [CODER]) => { rt.state.flybot ??= {}; rt.state.flybot.remote = { social: list } } + +async function grantToFriend(rt, botId, grantee = '@bob.home') { + const current = rt.grantsOf?.(botId) ?? [] + if (current.some(g => g.grantee === grantee)) return + await rt.setGrants(botId, [...current, { grantee }]) +} + +// A room of this home with one seat on another home, which is the surface the leak crossed. +async function withRemoteSeat(rt) { + const room = rt.state.rooms.war + room.participants = room.participants.filter(p => p.id !== 'chief-b') + room.members = room.members.filter(id => id !== 'chief-b') + publish(rt) + await grantToFriend(rt, 'chief-a') + await grantToFriend(rt, 'chief-b') + const invite = await rt.createInvitation({ createdByMemberId: 'owner-1' }) + const alice = await rt.redeemInvitation(invite.token, { displayName: 'Alice' }) + const actor = await rt.authenticate(alice.sessionToken) + await rt.addRoomParticipant('war', { kind: 'bot', remote: true, handle: '@coder.bob.home' }, actor) + return { room, actor } +} + +// The wire: the envelope the other home decodes. +const wire = (sent) => { + const last = sent.at(-1) + return last === undefined ? undefined : { to: last.to, ...parseRoomShare(last.text) } +} + +// A room line may name a person, a Bot and an attachment; it may not name this host's folders and it +// may not carry a credential — not even one that only looks like one (#2699). +const localDetail = [ + [/(?:^|[\s("'`=:,[)])\.?\/[A-Za-z]/u, 'an absolute path'], + [/ghp_[A-Za-z0-9]+/, 'a token-shaped string'], + [/token=/i, 'a token assignment'], +] + +function assertNoLocalDetail(text, dir, label = 'the room line') { + for (const [pattern, what] of localDetail) { + assert.doesNotMatch(String(text), pattern, `${label} must not carry ${what}: ${text}`) + } + if (dir !== undefined) assert.equal(String(text).includes(dir), false, `${label} names this home's folder: ${text}`) +} + +test('#2699: an unread image in a group names the attachment and its size, never this host\'s path', async (t) => { + const { rt, sent, dir } = host(t) + await rt.load() + await withRemoteSeat(rt) + + rt.config = { textOnlyModels: [] } + rt.catalog = async () => [] + rt.defaultRef = () => undefined + rt.ownRequest = () => false + installImages(rt) + + // The attachment store's layout, a real file so `attachmentFilePath` would answer a path. + const id = `sha256:${'ab'.repeat(32)}` + const objectPath = attachmentObjectPath(rt.home, id) + mkdirSync(join(objectPath, '..'), { recursive: true }) + writeFileSync(objectPath, 'fake-bytes') + const attachment = { attachmentId: id, name: 'holiday.png', mediaType: 'image/png', bytes: 9 } + + const [text] = await rt.imagesForGroup([attachment], 'war') + assert.match(text, /\[Image "holiday\.png" \(image\/png, 9 bytes\): it could not be read right now\./, 'the room still says which attachment and how big it is') + assert.equal(text.includes(dir), false, `the group placeholder names this home: ${text}`) + assertNoLocalDetail(text, dir, 'the group placeholder') + // The placeholder alone never names a path, even when one is offered: only the local reader may. + assertNoLocalDetail(imagePlaceholder(attachment, { path: objectPath, group: true }), dir, 'the group form with a path') + + sent.length = 0 + await rt.fanoutRoomLine(rt.state.rooms.war, { seq: 1, who: 'Alice', text, authorKind: 'human', authorId: 'owner-1' }) + const out = wire(sent) + assert.equal(out?.to, '@coder.bob.home', 'the line still reaches the far side') + assertNoLocalDetail(out.text, dir, "the friend's Bot") + assert.match(out.text, /holiday\.png/) + + // The reader that is this host may still be told where the bytes are (#779): a Bot chat keeps it. + const local = imagePlaceholder(attachment, { path: objectPath }) + assert.ok(local.includes(`"${objectPath}"`), local) +}) + +test('#2699: a crashed external Bot reports how it ended, and keeps its stderr in this host\'s log', async (t) => { + const { rt, sent, dir, warnings } = host(t) + await rt.load() + await withRemoteSeat(rt) + + const lines = [] + const realExternalRoomLine = rt.externalRoomLine + rt.externalRoomLine = async (room, line) => { + const stored = await realExternalRoomLine(room, line) + lines.push(stored) + return stored + } + + const room = rt.state.rooms.war + // A child that dies at startup: it names a file it could not open and prints a token-shaped string. + const CRASH = `process.stderr.write(${JSON.stringify(`Error: ENOENT: no such file or directory, open '${HOST_PATH}' token=${TOKEN}\n`)});setTimeout(() => process.exit(1), 10)` + const member = { id: 'chief-a', name: 'Chief A', external: { driver: 'subprocess', command: [process.execPath, '-e', CRASH], cwd: rt.home } } + const participant = createExternalRoomParticipant(rt) + t.after(() => participant.close()) + await participant.open({ room, member }) + + for (let i = 0; i < 60 && lines.length === 0; i += 1) await new Promise(r => setTimeout(r, 50)) + assert.equal(lines.length, 1, 'one crash line for the room') + const line = lines[0].text + assert.match(line, /crashed/, line) + assert.match(line, /\(exit 1\)/, `the room reads how the process ended: ${line}`) + assertNoLocalDetail(line, dir, 'the crash line') + + const out = wire(sent) + assert.equal(out?.to, '@coder.bob.home') + assertNoLocalDetail(out.text, dir, "the friend's Bot") + assert.match(out.text, /crashed/) + + // The diagnosis stays where the owner can read it and the far side cannot. + const logged = warnings.join('\n') + assert.ok(logged.includes(HOST_PATH), `the local log keeps the crash detail: ${logged}`) + assert.ok(logged.includes(TOKEN), 'the local log keeps what the child printed') +}) + +test('#2699: a process that cannot start says so to this host, not the room', async (t) => { + const { rt, sent, dir, warnings } = host(t) + await rt.load() + await withRemoteSeat(rt) + + const lines = [] + const realExternalRoomLine = rt.externalRoomLine + rt.externalRoomLine = async (room, line) => { + const stored = await realExternalRoomLine(room, line) + lines.push(stored) + return stored + } + + const room = rt.state.rooms.war + const command = join(dir, 'gone', 'agent-2699') + const member = { id: 'chief-a', name: 'Chief A', external: { driver: 'subprocess', command: [command] } } + const participant = createExternalRoomParticipant(rt) + t.after(() => participant.close()) + await participant.open({ room, member }) + + for (let i = 0; i < 60 && lines.length === 0; i += 1) await new Promise(r => setTimeout(r, 50)) + assert.equal(lines.length, 1, 'the room hears that it crashed') + assertNoLocalDetail(lines[0].text, dir, 'the crash line') + assert.equal(wire(sent)?.to, '@coder.bob.home') + assertNoLocalDetail(wire(sent).text, dir, "the friend's Bot") + assert.ok(warnings.join('\n').includes(command), 'this host\'s log names the path that could not be started') +}) + +// The one seam every stored room line passes, whichever writer made it (#1251): whatever the host +// redacts must not survive into the log or the wire. `rt.redactSecrets` is what `secrets.js` installs +// (`redactSecrets: text => redact(text)`); this fixture answers with the same shape. +test('#2699: every stored room line goes through the host redactor, whichever writer made it', async (t) => { + const { rt, sent } = host(t) + await rt.load() + const { room, actor } = await withRemoteSeat(rt) + rt.redactSecrets = text => String(text).replace(/ghp_[A-Za-z0-9]+/g, '[secret:GITHUB_TOKEN]') + + sent.length = 0 + await rt.postRoomMessage('war', { text: `the deploy key is ${TOKEN}, keep it out of the room` }, actor) + + const stored = (room.log ?? []).at(-1) + assert.match(stored.text, /\[secret:GITHUB_TOKEN\]/, `the stored line is redacted: ${stored.text}`) + assert.equal(stored.text.includes(TOKEN), false, stored.text) + const out = wire(sent) + assert.equal(out?.to, '@coder.bob.home') + assert.match(out.text, /\[secret:GITHUB_TOKEN\]/, out.text) + assert.equal(out.text.includes(TOKEN), false, "the friend's Bot never reads the value") +}) diff --git a/core/test/session-identity.test.js b/core/test/session-identity.test.js index b09a4547f..0aae9262f 100644 --- a/core/test/session-identity.test.js +++ b/core/test/session-identity.test.js @@ -217,8 +217,10 @@ test('the Session hooks pass DS Bot\'s own requests through, and still act on a // #779: with no vision model a picture cannot be read, and the placeholder then names the bytes on // disk so a Bot can pick them up with its own file tools. Both paths that turn an image into text -// for a text-only model share that placeholder: the group relay and a Bot chat. -test('an unread image names its bytes in a group and in a chat alike', async (t) => { +// for a text-only model share that placeholder: the group relay and a Bot chat. #2699: the path is +// for the reader on this host only — the group relay's text is a room line the room's other homes +// read, so it names the attachment by id and size and never a path of this machine. +test('an unread image names its bytes in a chat, and only a name in a group', async (t) => { const { ctx, rt, bench, home, stop } = start({ config: { textOnlyModels: ['deepseek/vision'] } }) t.after(stop) const id = 'b'.repeat(64) @@ -228,14 +230,16 @@ test('an unread image names its bytes in a group and in a chat alike', async (t) const picture = { attachmentId: `sha256:${id}`, name: 'chart.png', mediaType: 'image/png', bytes: 16 } const [line] = await rt.imagesForGroup([picture], 'r1') - assert.match(line, /it could not be read right now\. The file is at /) - assert.ok(line.includes(`"${objectPath}"`), line) + assert.match(line, /\[Image "chart\.png" \(image\/png, 16 bytes\): it could not be read right now\./, line) + assert.doesNotMatch(line, /The file is at/, 'the group line is read by other homes: no path of this host') + assert.equal(line.includes(home), false, line) bench.respond = () => 'It is a chart.' const message = { role: 'user', source: { kind: 'user' }, content: [{ type: 'image', attachment: picture }, { type: 'text', text: 'What is this?' }] } const chunks = await drain(ctx.llm.stream({ provider: 'deepseek', model: 'v4', sessionId: 'b2', messages: [message] })) assert.equal(chunks.at(-1).reason.kind, 'stop') - // No vision request went out: there is no model to send it to. + // No vision request went out: there is no model to send it to. The Bot's own chat is this host's + // reader, so its placeholder still names the bytes (#779). assert.deepEqual(bench.requests.map(request => request.model), ['v4']) assert.ok(bench.requests[0].messages[0].content[0].text.includes(`"${objectPath}"`), bench.requests[0].messages[0].content[0].text) }) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index afe266d1f..59541d4d9 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -191,7 +191,7 @@ context rollover for execution events. This internal DSH boundary adds no external backend or task-specific session mapping; see the [same-room participant contract](../interop/room-participants.md) for those gaps. -A member whose Bot record carries `external` is spoken for by a supervised subprocess instead of a DSH Session (#198). `room-participant.js` `createRoomParticipants` is the one boundary: it keeps the DSH driver for every other member and routes a member with that record — and later that member's own session id — to `room-participant-external.js`. The record is a typed block, `external: { driver: 'subprocess', command: [argv...], cwd?, env? }`, written through `roster.js` `cleanExternal`, which refuses a string command (a shared room setting that runs a shell line is an injection surface), any driver but `subprocess`, and an interpreter as `command[0]` (`sh`, `bash`, `pwsh`, `env` and the like): an argv array whose first word is a shell is the same shell line spelled differently. One process is supervised per (room, Bot) and talks one JSON object per line on stdio (`{ type: 'turn', text, note, from, exchangeId }` in, `{ text }` or `{ error }` out), so the same external Bot can be invited to a second room as a second process over one identity. `open`, `runTurn`, `snapshot` and `cancel` reach each driver through the router. 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 (`groups.js` `externalRoomLine`), and a turn that reaches a dead process ends as `{ failure }` instead of silence. A process that died is not a dead end: the next turn in that room starts a fresh one, which is what the room's line promises. A stop asks on the protocol first (`{ type: 'stop' }`), sends `SIGTERM` after a short grace and `SIGKILL` after a longer one, so a child that ignores the request and the signal does not outlive its seat; an entry whose stop is still in flight is never reused for the next turn, and a turn the user stopped ends as nothing to answer rather than a failure. 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 itself; a timeout reads as `timed out`. A timed-out turn ends the child the same way a stopped one does — the protocol stop line, then `SIGTERM`, then `SIGKILL` — because the protocol carries no turn id: left running, its late answer would settle whichever turn is open then, which is the next one, so that turn would read the previous turn's text ([#2634](https://github.com/ThinkFlowLab/FlyBot/issues/2634)). The next turn therefore starts a fresh process. The seat owns the process: `store.js` `dropGroupSession` closes the `(room, Bot)` process when a member is removed or the Bot is deleted, and `groups.js` `install` closes the rest on host teardown (`ctx.effect`). stdout is read as lines, and only a newline-terminated line is a message: a line that outgrows the 4 KB in-flight buffer is that turn's failure (`answer exceeded 4096 bytes`), not a timeout: only its bytes are dropped up to the newline, the process stays for the next turn because the child did answer, and the room gets one line saying the answer was too large, while a long line written with no turn open is discarded to its newline and never parsed, and a turn opened while such a log line's tail is still in flight waits for its own answer instead of being settled by it ([#2689](https://github.com/ThinkFlowLab/FlyBot/issues/2689)). +A member whose Bot record carries `external` is spoken for by a supervised subprocess instead of a DSH Session (#198). `room-participant.js` `createRoomParticipants` is the one boundary: it keeps the DSH driver for every other member and routes a member with that record — and later that member's own session id — to `room-participant-external.js`. The record is a typed block, `external: { driver: 'subprocess', command: [argv...], cwd?, env? }`, written through `roster.js` `cleanExternal`, which refuses a string command (a shared room setting that runs a shell line is an injection surface), any driver but `subprocess`, and an interpreter as `command[0]` (`sh`, `bash`, `pwsh`, `env` and the like): an argv array whose first word is a shell is the same shell line spelled differently. One process is supervised per (room, Bot) and talks one JSON object per line on stdio (`{ type: 'turn', text, note, from, exchangeId }` in, `{ text }` or `{ error }` out), so the same external Bot can be invited to a second room as a second process over one identity. `open`, `runTurn`, `snapshot` and `cancel` reach each driver through the router. 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 (`groups.js` `externalRoomLine`), and a turn that reaches a dead process ends as `{ failure }` instead of silence. A process that died is not a dead end: the next turn in that room starts a fresh one, which is what the room's line promises. A stop asks on the protocol first (`{ type: 'stop' }`), sends `SIGTERM` after a short grace and `SIGKILL` after a longer one, so a child that ignores the request and the signal does not outlive its seat; an entry whose stop is still in flight is never reused for the next turn, and a turn the user stopped ends as nothing to answer rather than a failure. A crash line reports how the process ended and nothing else — `exit ` or `signal ` — because that line is a room line and the room may hold other homes (#2699): the child's last ~200 characters of `stderr`, flattened to one line, go to `rt.warn` (this host's log, where the owner reads them) and never into the room, since a child that dies at startup names the file it could not open and may have printed a token. A spawn that fails reports no reason at all to the room, only that it crashed. A timeout reads as `timed out`. A timed-out turn ends the child the same way a stopped one does — the protocol stop line, then `SIGTERM`, then `SIGKILL` — because the protocol carries no turn id: left running, its late answer would settle whichever turn is open then, which is the next one, so that turn would read the previous turn's text ([#2634](https://github.com/ThinkFlowLab/FlyBot/issues/2634)). The next turn therefore starts a fresh process. The seat owns the process: `store.js` `dropGroupSession` closes the `(room, Bot)` process when a member is removed or the Bot is deleted, and `groups.js` `install` closes the rest on host teardown (`ctx.effect`). stdout is read as lines, and only a newline-terminated line is a message: a line that outgrows the 4 KB in-flight buffer is that turn's failure (`answer exceeded 4096 bytes`), not a timeout: only its bytes are dropped up to the newline, the process stays for the next turn because the child did answer, and the room gets one line saying the answer was too large, while a long line written with no turn open is discarded to its newline and never parsed, and a turn opened while such a log line's tail is still in flight waits for its own answer instead of being settled by it ([#2689](https://github.com/ThinkFlowLab/FlyBot/issues/2689)). ### Group standing section (#894) @@ -199,7 +199,7 @@ A member's group Session reads the group's fixed rules from a system-prompt sect ### Room context budget (#1203) -A group turn does not hand the model the raw ring buffer. `src/host/room-context.js` `fitRoomLines` takes the lines the count caps already chose (`ROOM_FIRST_LOOK` 12, `ROOM_DELTA_MAX` 24, and `ROOM_EARLIER_MAX` 8 of those from before the round) and keeps a newer-first slice of at most `ROOM_CONTEXT_BUDGET` characters (4000). The budget is characters because the measured essays are mostly CJK, about one token per character. A long bot line that reads as CI, merge-queue, or review narration (`statusShaped`: at least 400 characters and two of the status hints) is collapsed to a one-line stub that keeps the opening and up to four `@` names. Since #1251 every writer of `room.log` stores that same stub rather than only the turn: `groups.js` `appendRoom` (a group round) and `room-membership.js` `append` (`postRoomMessage`, a seat's `line` op, a face-room event) both pass the stamped line through `prepareRoomLine`, so the stored line and the next turn cannot disagree. A person's line (`botId` absent), a `kind`-carrying event line **from this home** and a short handoff come back unchanged and are stored verbatim. A fenced peer line (#98) is compacted *inside* its fence and wrapped again with the same `from` name, because compacting the fenced string would strip the markers and store a guest's text as this home's own; the fence decides *before* the `kind`/`botId` exemptions, because both travel from the other home (`room-membership.js` copies the peer's `kind`), so a peer could otherwise mark its essay `notice` or type it as a person and store it whole. A long Bot line that is not status-shaped is still stored whole — the store gate reuses `#1204`'s rule, it does not widen it. When lines are left out, the turn says so and points at `read_group_chat` for an exact earlier line; `read_group_chat` returns the stored line, which for a Bot essay is the stub. Every character bound in this module goes through `shared/text-boundary.js` `cut`, never a bare `slice`: the stub's hundred units (`compactStatus`), the transcript clip at `room - 1` in `fitRoomLines`, and the card, board and presence fields `oneLine` caps. A bound that lands between the halves of a surrogate pair drops the pair whole, so neither `room.log` nor a rendered turn carries a lone surrogate the encoder would turn into U+FFFD ([#1960](https://github.com/ThinkFlowLab/FlyBot/issues/1960)). Covered by `core/test/group-work.test.js`, `core/test/room-membership.test.js` and `core/test/remote-room.test.js` (the stores) and `core/test/room-context.test.js` (the rule). +A group turn does not hand the model the raw ring buffer. `src/host/room-context.js` `fitRoomLines` takes the lines the count caps already chose (`ROOM_FIRST_LOOK` 12, `ROOM_DELTA_MAX` 24, and `ROOM_EARLIER_MAX` 8 of those from before the round) and keeps a newer-first slice of at most `ROOM_CONTEXT_BUDGET` characters (4000). The budget is characters because the measured essays are mostly CJK, about one token per character. A long bot line that reads as CI, merge-queue, or review narration (`statusShaped`: at least 400 characters and two of the status hints) is collapsed to a one-line stub that keeps the opening and up to four `@` names. Since #1251 every writer of `room.log` stores that same stub rather than only the turn: `groups.js` `appendRoom` (a group round) and `room-membership.js` `append` (`postRoomMessage`, a seat's `line` op, a face-room event) both pass the stamped line through `prepareRoomLine`, so the stored line and the next turn cannot disagree. That gate also runs the host's secret redaction (`rt.redactSecrets`, wired once at install through `setRoomLineRedactor`) over every line before it is stored: the log is fanned out to the room's other homes (`fanoutRoomLine`), so a pasted key never reaches the wire or the stored log, whichever writer made the line (#2699). A person's line (`botId` absent), a `kind`-carrying event line **from this home** and a short handoff come back unchanged and are stored verbatim. A fenced peer line (#98) is compacted *inside* its fence and wrapped again with the same `from` name, because compacting the fenced string would strip the markers and store a guest's text as this home's own; the fence decides *before* the `kind`/`botId` exemptions, because both travel from the other home (`room-membership.js` copies the peer's `kind`), so a peer could otherwise mark its essay `notice` or type it as a person and store it whole. A long Bot line that is not status-shaped is still stored whole — the store gate reuses `#1204`'s rule, it does not widen it. When lines are left out, the turn says so and points at `read_group_chat` for an exact earlier line; `read_group_chat` returns the stored line, which for a Bot essay is the stub. Every character bound in this module goes through `shared/text-boundary.js` `cut`, never a bare `slice`: the stub's hundred units (`compactStatus`), the transcript clip at `room - 1` in `fitRoomLines`, and the card, board and presence fields `oneLine` caps. A bound that lands between the halves of a surrogate pair drops the pair whole, so neither `room.log` nor a rendered turn carries a lone surrogate the encoder would turn into U+FFFD ([#1960](https://github.com/ThinkFlowLab/FlyBot/issues/1960)). Covered by `core/test/group-work.test.js`, `core/test/room-membership.test.js` and `core/test/remote-room.test.js` (the stores) and `core/test/room-context.test.js` (the rule). The open task board for that room is a compact snapshot in the same turn (`formatRoomBoard`): the newest overall task, then its subtasks and any room card that is not under a different overall task, up to eight cards. Done cards stay off. Titles and notes are one line, capped, with `<` and `>` removed. An unclaimed queue item reads `unassigned [open 3h Dev]` — its role and its wait on the card itself — and the snapshot ends with one line per role that has something waiting (`- Queue Dev: 2 waiting, oldest 3h`), so starvation is visible without reading every card (#1247). A role with nothing waiting is omitted. A face-to-face room has no snapshot and keeps `@Name` as how its lead gives a part. The standing section does not carry the snapshot or the transcript, so it still changes only when the group settings change. In a group that has a board, `ROOM_REPLY_BOARD` and the task-lead note say assigning, claiming, and progress are `track_tasks` writes (overall task, subtask, assignee, column, progress), `@Name` only wakes, and a status the board already shows is `[PASS]`. A task-drive wake in the group says to set that card's column and progress, and appends the same snapshot after its cue. Covered by `core/test/room-context.test.js`, `core/test/rounds.test.js`, and `core/test/task-drive.test.js`. @@ -621,8 +621,9 @@ A claim works the same way (#1247's compare-and-set and WIP = 1, unchanged): `tr - When no model can read an image, both places it becomes text — a Bot chat (`imageAsText` through `withImagesAsText`) and the group relay (`imagesForGroup`) — use the same `imagePlaceholder`. - That placeholder names the file when the Bot's own file tools can open it: `attachmentFilePath` takes the attachment store's `imageHostPath` through the file sandbox's `processPathFromHostPath` (the same mapping `dsh-spill-policy` applies to spooled tool output) and falls back to the local object layout `/attachments/v1/objects//`. Either candidate must `stat` as a regular file, so a Bot is never sent after a file that is not there (a host path the sandbox cannot open is worse than no path), and only a `sha256:<64 hex>` id has an object path at all. +- **A room line carries no path of this host (#2699).** The group form of the placeholder (`group: true`, what `imagesForGroup` returns) never names the path, even when one is passed in: it identifies the picture by its file name — or the `sha256:` prefix when it has none — its media type and its size, and says it could not be read and to ask the user. That text is a room line, which `fanoutRoomLine` sends to the room's other homes, where this machine's folders mean nothing and name the host to a friend's Bot; the own-chat form keeps the path, because its reader is this home's model and the Bot's own file tools can open it (#779). - Nothing is copied into a Bot's working folder: a copy would need a lifecycle and a cleanup policy of its own, and the readable path already lets a Bot move or submit the bytes itself. -- Covered by `core/test/images.test.js` and, through the Session hook and the group relay, `core/test/session-identity.test.js`. +- Covered by `core/test/images.test.js`, `core/test/room-line-leak.test.js` (the bytes a remote seat receives) and, through the Session hook and the group relay, `core/test/session-identity.test.js`. ### The Project board and who may act on it (#1710) diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 23245c27a..16459e958 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -5706,7 +5706,7 @@ diff --git a/src/host/images.js b/src/host/images.js import { splitRef } from './text.js' export function offloadTargets(session, beforeSeq) { -@@ -27,6 +27,56 @@ export function offloadTargets(session, beforeSeq) { +@@ -27,6 +27,58 @@ export function offloadTargets(session, beforeSeq) { return targets } @@ -5753,9 +5753,11 @@ diff --git a/src/host/images.js b/src/host/images.js +} + +// One placeholder for both places an unread image becomes text: a Bot chat (`imageAsText`) and the -+// group relay (`imagesForGroup`). It names the bytes only when `path` is a file this Bot can open. ++// group relay (`imagesForGroup`). It names the bytes only when `path` is a file this Bot can open, and ++// never in the group form: a group line is fanned out to the room's other homes (#2699), where a path ++// of this host says nothing the far side can open and names this machine's folders to it. +export function imagePlaceholder(attachment, { path, group = false } = {}) { -+ const where = path === undefined ? '' : ` The file is at "${path}"; your own file tools can open it.` ++ const where = group || path === undefined ? '' : ` The file is at "${path}"; your own file tools can open it.` + const cannot = group ? '' : 'you cannot see images and ' + return `[Image ${imageLabel(attachment)}${imageFacts(attachment)}: ${cannot}it could not be read right now.${where} Ask the user what it shows if it matters.]` +} @@ -5809,7 +5811,7 @@ diff --git a/src/host/images.js b/src/host/images.js const label = imageLabel(attachment) return reading === null - ? `[Image ${label}: it could not be read right now. Ask the user what it shows if it matters.]` -+ ? imagePlaceholder(attachment, { path: await attachmentFilePath(attachment, attachmentServices()), group: true }) ++ ? imagePlaceholder(attachment, { group: true }) : `[Image ${label}, read by ${reading.model}:]\n${reading.text}\n[End of image ${label}]` })) } @@ -12051,7 +12053,7 @@ diff --git a/test/images.test.js b/test/images.test.js test('offloadTargets selects live images from prior user and tool messages', () => { const events = [ -@@ -32,3 +32,170 @@ test('offloadTargets selects live images from prior user and tool messages', () +@@ -32,3 +32,173 @@ test('offloadTargets selects live images from prior user and tool messages', () { seq: 2, imageIndexes: [0] }, ]) }) @@ -12131,6 +12133,9 @@ diff --git a/test/images.test.js b/test/images.test.js + const group = imagePlaceholder(attachment, { path: '/p/x', group: true }) + assert.match(group, /\[Image "IMG_2431\.jpeg" \(image\/jpeg, 1080x1596, 213569 bytes\): it could not be read right now\./) + assert.doesNotMatch(group, /you cannot see images/) ++ // #2699: the group line is fanned out to the room's other homes, so it never names a path of this ++ // host — not even when one is passed in. ++ assert.doesNotMatch(group, /The file is at/, 'a group line is read on the far side: no path of this host') + + // An unnamed image keeps the sha prefix the host has always shown. + assert.match(imagePlaceholder({ attachmentId: id }), /\[Image sha256:bdc77c70:/) @@ -15429,15 +15434,17 @@ diff --git a/test/session-identity.test.js b/test/session-identity.test.js test('an image in a Bot chat is read for that chat, and the chat gets the reading', async (t) => { const { ctx, bench, stop } = start() t.after(stop) -@@ -172,3 +172,43 @@ test('the Session hooks pass DS Bot\'s own requests through, and still act on a +@@ -172,3 +172,47 @@ test('the Session hooks pass DS Bot\'s own requests through, and still act on a assert.equal(images(own('b2', picture), pass), passed) assert.notEqual(images(round('b2', picture), pass), passed) }) + +// #779: with no vision model a picture cannot be read, and the placeholder then names the bytes on +// disk so a Bot can pick them up with its own file tools. Both paths that turn an image into text -+// for a text-only model share that placeholder: the group relay and a Bot chat. -+test('an unread image names its bytes in a group and in a chat alike', async (t) => { ++// for a text-only model share that placeholder: the group relay and a Bot chat. #2699: the path is ++// for the reader on this host only — the group relay's text is a room line the room's other homes ++// read, so it names the attachment by id and size and never a path of this machine. ++test('an unread image names its bytes in a chat, and only a name in a group', async (t) => { + const { ctx, rt, bench, home, stop } = start({ config: { textOnlyModels: ['deepseek/vision'] } }) + t.after(stop) + const id = 'b'.repeat(64) @@ -15447,14 +15454,16 @@ diff --git a/test/session-identity.test.js b/test/session-identity.test.js + const picture = { attachmentId: `sha256:${id}`, name: 'chart.png', mediaType: 'image/png', bytes: 16 } + + const [line] = await rt.imagesForGroup([picture], 'r1') -+ assert.match(line, /it could not be read right now\. The file is at /) -+ assert.ok(line.includes(`"${objectPath}"`), line) ++ assert.match(line, /\[Image "chart\.png" \(image\/png, 16 bytes\): it could not be read right now\./, line) ++ assert.doesNotMatch(line, /The file is at/, 'the group line is read by other homes: no path of this host') ++ assert.equal(line.includes(home), false, line) + + bench.respond = () => 'It is a chart.' + const message = { role: 'user', source: { kind: 'user' }, content: [{ type: 'image', attachment: picture }, { type: 'text', text: 'What is this?' }] } + const chunks = await drain(ctx.llm.stream({ provider: 'deepseek', model: 'v4', sessionId: 'b2', messages: [message] })) + assert.equal(chunks.at(-1).reason.kind, 'stop') -+ // No vision request went out: there is no model to send it to. ++ // No vision request went out: there is no model to send it to. The Bot's own chat is this host's ++ // reader, so its placeholder still names the bytes (#779). + assert.deepEqual(bench.requests.map(request => request.model), ['v4']) + assert.ok(bench.requests[0].messages[0].content[0].text.includes(`"${objectPath}"`), bench.requests[0].messages[0].content[0].text) +}) diff --git a/patches/ds-bot/0012-room-membership.patch b/patches/ds-bot/0012-room-membership.patch index 6e3e95313..caf6c394d 100644 --- a/patches/ds-bot/0012-room-membership.patch +++ b/patches/ds-bot/0012-room-membership.patch @@ -1085,7 +1085,7 @@ diff --git a/src/host/groups.js b/src/host/groups.js +import { faceMentions } from './face-mentions.js' +import { cleanName, stripSessionChips, unwrapUntrusted } from './delivery.js' +import { createRoomParticipants } from './room-participant.js' -+import { ROOM_CONTEXT_BUDGET, boardCoversCue, fitRoomLines, formatRoomBoard, formatRoomPresence, prepareRoomLine } from './room-context.js' ++import { ROOM_CONTEXT_BUDGET, boardCoversCue, fitRoomLines, formatRoomBoard, formatRoomPresence, prepareRoomLine, setRoomLineRedactor } from './room-context.js' +import { workDispatch } from './work-dispatch.js' +import { retiredBot } from './store.js' + @@ -1235,7 +1235,7 @@ diff --git a/src/host/groups.js b/src/host/groups.js if (slot.status === 'missed') return `${slot.name} (no answer)` if (slot.status === 'working') return `${slot.name} (working on it; posts when done)` if (slot.status === 'waiting-approval') return `${slot.name} (waiting for the user's approval; posts when done)` -@@ -100,162 +100,247 @@ const slotLabel = (slot) => { +@@ -100,162 +100,250 @@ const slotLabel = (slot) => { return slot.name } @@ -1262,6 +1262,9 @@ diff --git a/src/host/groups.js b/src/host/groups.js + const participant = createRoomParticipants(rt) + fanoutRoomLine = (room, line) => rt.fanoutRoomLine?.(room, line) + stampRoomLine = (room, line) => rt.stampHop?.(room, line) ?? line ++ // A stored room line leaves for the room's other homes, so the store gate redacts with this host's ++ // own redactor (#2699). ++ setRoomLineRedactor(text => rt.redactSecrets?.(text) ?? text) + rt.notePostedLine = (roomId, text) => postedLines.set(roomId, [...(postedLines.get(roomId) ?? []), text].slice(-20)) + // What an external participant's process does is a room line (#198), so a crash is seen where + // the Bot speaks and not only in a log nobody reads. diff --git a/patches/ds-bot/files/src/host/room-context.js b/patches/ds-bot/files/src/host/room-context.js index f37fc5f6f..cbd891405 100644 --- a/patches/ds-bot/files/src/host/room-context.js +++ b/patches/ds-bot/files/src/host/room-context.js @@ -105,9 +105,20 @@ const FENCED_LINE = /^\n?([\s\S]*?)\n?<\/untru // Bot speech only. A person's line, and a short line, stay as stored. The log // object is not modified: a collapsed line is a copy. -export function prepareRoomLine(line) { +// +// Every writer of `room.log` passes through here — `groups.js` `appendRoom` (a group round) and +// `room-membership.js` `append` (a person's line, a seat's `line` op, a face-room event) — and the log +// is fanned out to the room's other homes (`fanoutRoomLine`), so the redaction this host trusts runs +// once, on this gate, instead of in each builder of a line (#2699). `redact` is that host's +// `rt.redactSecrets`, wired once at install; a line the redactor leaves alone comes back unchanged. +let redactRoomLine = text => text +export function setRoomLineRedactor(redact) { redactRoomLine = redact } + +export function prepareRoomLine(line, { redact = redactRoomLine } = {}) { if (line == null || typeof line !== 'object') return line if (typeof line.text !== 'string') return line + const clean = typeof redact === 'function' ? redact(line.text) : line.text + if (clean !== line.text) line = { ...line, text: clean } // A line a writer already fenced for another home's Bot keeps its fence, and while it is fenced // neither `kind` nor `authorKind` is an exemption: both travel from the other home // (room-membership.js copies them), so a peer could otherwise mark an essay `notice` or type it as a diff --git a/patches/ds-bot/files/src/host/room-membership.js b/patches/ds-bot/files/src/host/room-membership.js index 54543b51f..82d3764be 100644 --- a/patches/ds-bot/files/src/host/room-membership.js +++ b/patches/ds-bot/files/src/host/room-membership.js @@ -413,7 +413,7 @@ export function install(rt) { // one (postRoomMessage, a seat's `line` op, a face-room event), so the same rule runs here. The rule // itself knows a fence now (`room-context.js` `prepareRoomLine`): it collapses the inside and wraps it // again, so a guest's essay is never stored as this home's own text, and both writers share one copy. - const storeLine = (line) => prepareRoomLine(line) + const storeLine = (line) => prepareRoomLine(line, { redact: rt.redactSecrets }) const append = (room, line) => { ensureLog(room) diff --git a/patches/ds-bot/files/src/host/room-participant-external.js b/patches/ds-bot/files/src/host/room-participant-external.js index 5a83c87a4..9f852ad2d 100644 --- a/patches/ds-bot/files/src/host/room-participant-external.js +++ b/patches/ds-bot/files/src/host/room-participant-external.js @@ -81,12 +81,14 @@ export function createExternalRoomParticipant(rt) { return `${member.name}'s external process ${state === 'timeout' ? 'timed out' : state}${detail ? ` (${detail})` : ''}.` } - // A crash line carries the child's last words, flattened and capped, so `exit 1` is diagnosable - // from the room itself (#1164). + // The child's last words, flattened and capped, are this host's diagnosis (#1164). const tailOf = (entry) => String(entry.stderr ?? '').slice(-STDERR_TAIL).replace(/[\r\n]+/g, ' ').trim().slice(0, STDERR_TAIL) // The room sees what happened to its participant, not just a turn that never came back (#198). - async function report(room, member, state, detail) { - rt.warn?.('ds-bot: external Bot %s in %s: %s%s', member.name, room.name, state, detail ? ` (${detail})` : '') + // `roomDetail` is the part the room may read — an exit code, a signal name, a timeout reason — and + // `logDetail` is this host's diagnosis (the stderr tail, a spawn error naming a file): a room line is + // fanned out to the room's other homes, so only the log keeps the detail (#2699). + async function report(room, member, state, roomDetail, logDetail = roomDetail) { + rt.warn?.('ds-bot: external Bot %s in %s: %s%s', member.name, room.name, state, logDetail ? ` (${logDetail})` : '') try { await rt.externalRoomLine?.(room, { who: member.name, @@ -94,7 +96,7 @@ export function createExternalRoomParticipant(rt) { authorId: member.id, authorKind: 'bot', kind: 'external', - text: stateLine(member, state, detail), + text: stateLine(member, state, roomDetail), }) } catch (error) { rt.warn?.('ds-bot: the state of %s in %s was not posted: %s', member.name, room.name, error) @@ -148,7 +150,7 @@ export function createExternalRoomParticipant(rt) { }) } catch (error) { settle(entry, 'crashed', oneLine(error?.message ?? error)) - void report(room, member, 'crashed', entry.detail) + void report(room, member, 'crashed', undefined, entry.detail) return entry } entry.child = child @@ -192,7 +194,7 @@ export function createExternalRoomParticipant(rt) { entry.child = undefined entry.reported = true settle(entry, 'crashed', oneLine(error?.message ?? error)) - void report(room, member, 'crashed', entry.detail) + void report(room, member, 'crashed', undefined, entry.detail) }) const reportExit = (code, signal) => { // One line either way: whichever of 'exit' and 'close' reports first wins (#1164). @@ -202,11 +204,12 @@ export function createExternalRoomParticipant(rt) { // room should hear about: it stopped when asked, so no 'crashed' line (#198). if (entry.state === 'stopped' || entry.stopping === true) return const clean = code === 0 - const detail = signal !== null ? `signal ${signal}` : `exit ${code}` + const ending = signal !== null ? `signal ${signal}` : `exit ${code}` const tail = clean ? '' : tailOf(entry) - const said = tail === '' ? detail : `${detail}: ${tail}` - settle(entry, clean ? 'exited' : 'crashed', said) - void report(room, member, entry.state, said) + // The room hears how the process ended; its last words stay in the log, where the owner reads + // them and a seat on another home cannot (#2699). + settle(entry, clean ? 'exited' : 'crashed', tail === '' ? ending : `${ending}: ${tail}`) + void report(room, member, entry.state, ending, entry.detail) entry.stopping = false } // 'exit' must free the seat at once — otherwise the next turn writes into a dead child and the @@ -339,7 +342,7 @@ export function createExternalRoomParticipant(rt) { entry.child.stdin.write(`${JSON.stringify({ type: 'turn', text, note, ...(by ? { from: by } : {}), exchangeId })}\n`) } catch (error) { settle(entry, 'crashed', oneLine(error?.message ?? error)) - void report(room, member, 'crashed', entry.detail) + void report(room, member, 'crashed', undefined, entry.detail) } return done } diff --git a/patches/ds-bot/files/test/room-context.test.js b/patches/ds-bot/files/test/room-context.test.js index c850c851c..f6b226c62 100644 --- a/patches/ds-bot/files/test/room-context.test.js +++ b/patches/ds-bot/files/test/room-context.test.js @@ -36,6 +36,25 @@ test('prepare collapses bot status and leaves the stored line and a person line' assert.equal(prepareRoomLine(short), short) }) +test('the store gate runs the host redactor on every line, and leaves a clean one alone (#2699)', () => { + // A room line leaves for the room's other homes (`fanoutRoomLine`), so the one gate both writers of + // `room.log` pass through is where the host's redactor runs (`rt.redactSecrets`, a stub here). + const redact = text => text.replace(/ghp_[A-Za-z0-9]+/g, '[secret:GITHUB_TOKEN]') + const person = { seq: 1, who: 'User', text: 'the key is ghp_FAKE2699, keep it here' } + const stored = prepareRoomLine(person, { redact }) + assert.match(stored.text, /\[secret:GITHUB_TOKEN\]/) + assert.equal(person.text.includes('ghp_FAKE2699'), true, 'the log line is not rewritten') + const bot = { seq: 2, who: 'Dev', botId: 'dev', text: 'ghp_FAKE2699 is what I pasted' } + assert.match(prepareRoomLine(bot, { redact }).text, /\[secret:GITHUB_TOKEN\]/) + // A bot essay is collapsed after the redaction, so a secret in it is gone whichever end it was on. + const essayBot = { seq: 3, who: 'Dev', botId: 'dev', text: essay('ghp_FAKE2699') } + assert.doesNotMatch(prepareRoomLine(essayBot, { redact }).text, /ghp_FAKE/) + const clean = { seq: 4, who: 'User', text: 'nothing to redact here' } + assert.equal(prepareRoomLine(clean, { redact }), clean, 'a line with no secret keeps its identity') + // Without the host's redactor (a caller that has none) the line is prepared as before. + assert.equal(prepareRoomLine(person), person) +}) + test('the transcript stays inside the budget and prefers a newer short line over an older essay', () => { const lines = [] for (let seq = 1; seq <= 8; seq += 1) lines.push({ seq, who: 'Dev', botId: 'dev', text: essay(`marker-${seq}`) }) diff --git a/patches/ds-bot/files/test/room-external.test.js b/patches/ds-bot/files/test/room-external.test.js index 4bd203597..8ea0d5d79 100644 --- a/patches/ds-bot/files/test/room-external.test.js +++ b/patches/ds-bot/files/test/room-external.test.js @@ -11,10 +11,11 @@ const DIE = 'process.exit(3)' function fixture(t) { const lines = [] + const warnings = [] const rt = { live: new Map(), groupOwners: new Map(), roomWaiters: new Map(), workDirOf: () => undefined, - warn: () => {}, + warn: (...args) => warnings.push(args.join(' ')), externalRoomLine: async (room, line) => { lines.push({ roomId: room.id, ...line }); return line }, } const driver = createExternalRoomParticipant(rt) @@ -25,7 +26,7 @@ function fixture(t) { room, member, sessionId: `external:ops:${member.id}`, text, note: 'your turn', signal: controller.signal, started: [], by: 'chief', exchangeId: `x-${member.id}`, }) - return { driver, rt, room, lines, run } + return { driver, rt, room, lines, warnings, run } } test('an external Bot whose process dies answers with a failure and lands in the room, not in silence (#198)', async (t) => { @@ -233,20 +234,27 @@ test('the timeout state line says timed out (#1164)', async (t) => { assert.doesNotMatch(entry.text, /process timeout/) }) -test('a crash line carries the last words from stderr (#1164)', async (t) => { +test('a crash line reports the ending, and the last words stay in this host\'s log (#1164, #2699)', async (t) => { const BOOM = "process.stderr.write('boom: missing config\\n',()=>process.exit(3))" - const { lines, run } = fixture(t) + const { lines, warnings, run } = fixture(t) const member = { id: 'boom', name: 'Boom', external: { driver: 'subprocess', command: [process.execPath, '-e', BOOM] } } assert.equal((await run(member))?.failure?.reason, 'crashed') const entry = await waitForLine(lines, 'boom') - assert.match(entry.text, /crashed \(exit 3: boom: missing config\)/) + assert.match(entry.text, /crashed \(exit 3\)/) + // The room reads how the process ended; its last words name this host's files, and the room's line + // is read by the other homes in it (#2699). + assert.doesNotMatch(entry.text, /missing config/) + assert.ok(warnings.some(line => line.includes('boom: missing config')), 'the log keeps what the child said') // The tail is capped: 400 characters of stderr keep only the last 200. const NOISE = "process.stderr.write('x'.repeat(400),()=>process.exit(3))" const noisy = { id: 'noise', name: 'Noise', external: { driver: 'subprocess', command: [process.execPath, '-e', NOISE] } } assert.equal((await run(noisy))?.failure?.reason, 'crashed') const noise = await waitForLine(lines, 'noise') - assert.match(noise.text, /x{200}/) - assert.doesNotMatch(noise.text, /x{201}/) + assert.match(noise.text, /crashed \(exit 3\)/) + assert.doesNotMatch(noise.text, /x{2,}/) + const logged = warnings.find(line => line.includes('xxxxxxxx')) ?? '' + assert.match(logged, /x{200}/) + assert.doesNotMatch(logged, /x{201}/) }) test('a crash is reported when a grandchild holds the stdio open (#1164)', async (t) => { // On the first launch the child leaves a grandchild on its inherited stdio and exits 1, so @@ -256,14 +264,16 @@ test('a crash is reported when a grandchild holds the stdio open (#1164)', async const HELD = "const fs=require('node:fs');const cp=require('node:child_process');if(!fs.existsSync(process.env.MARK)){fs.writeFileSync(process.env.MARK,'1');fs.writeSync(2,'boom: helper holds the pipe\\n');cp.spawn(process.execPath,['-e','setTimeout(()=>{},1200)'],{stdio:'inherit'});process.exit(1)}let b='';process.stdin.setEncoding('utf8');process.stdin.on('data',c=>{b+=c;let i;while((i=b.indexOf('\\n'))!==-1){const l=b.slice(0,i);b=b.slice(i+1);const m=JSON.parse(l);if(m.type==='turn')process.stdout.write(JSON.stringify({text:'again: '+m.text})+'\\n')}})" const dir = mkdtempSync(join(tmpdir(), 'flybot-external-')) const mark = join(dir, 'mark') - const { lines, run } = fixture(t) + const { lines, warnings, run } = fixture(t) const member = { id: 'held', name: 'Held', external: { driver: 'subprocess', command: [process.execPath, '-e', HELD], env: { MARK: mark } } } const started = Date.now() const first = run(member) // 900 ms is before the grandchild's 1.2 s: only reporting from 'exit' can meet this. const entry = await waitForLine(lines, 'held', 900) const elapsed = Date.now() - started - assert.match(entry.text, /crashed \(exit 1: boom: helper holds the pipe\)/) + assert.match(entry.text, /crashed \(exit 1\)/) + assert.doesNotMatch(entry.text, /helper holds the pipe/) + assert.ok(warnings.some(line => line.includes('helper holds the pipe')), 'the log keeps what the child said') assert.ok(elapsed < 900, `the crash line waited for close (${elapsed} ms)`) assert.equal((await first)?.failure?.reason, 'crashed') // The seat was cleared on 'exit', so the next turn reaches a fresh process, not a dead pipe. diff --git a/patches/ds-bot/files/test/room-line-leak.test.js b/patches/ds-bot/files/test/room-line-leak.test.js new file mode 100644 index 000000000..1b69e3ca2 --- /dev/null +++ b/patches/ds-bot/files/test/room-line-leak.test.js @@ -0,0 +1,273 @@ +// #2699: a room line is not local. `spawnShare` fans every stored line to the room's remote seats +// (`room-membership.js` `fanoutRoomLine`), so whatever this host writes into a line is read on the +// other side. Two builders used to put this host's own detail there: the unread-image placeholder +// named the attachment's absolute path, and a crashed external process's line carried the last 200 +// bytes of the child's stderr (a child that dies at startup names the file it could not open, and +// may have printed a token). What is asserted below is the decoded room-share envelope the far side +// receives — never the local log, which may keep the detail for the owner. +import assert from 'node:assert/strict' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { install as installStore } from '../src/host/store.js' +import { install as installMemberAuth } from '../src/host/member-auth.js' +import { install as installAuthz } from '../src/host/authz.js' +import { install as installRooms } from '../src/host/room-membership.js' +import { install as installReach } from '../src/host/reach.js' +import { install as installRoster } from '../src/host/roster.js' +import { install as installGroups } from '../src/host/groups.js' +import { attachmentObjectPath, imagePlaceholder, install as installImages } from '../src/host/images.js' +import { createExternalRoomParticipant } from '../src/host/room-participant-external.js' +import { parseRoomShare } from '../src/shared/room-share.js' + +// What the child could not open — it only has to look like this host's layout. +const HOST_PATH = '/data/dsh/bot/home/state.json' +const TOKEN = 'ghp_FAKE2699FAKE2699FAKE2699FAKE2699' + +const TEAM = { + version: 2, + revision: 1, + mainBotIds: ['chief-a'], + members: { 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'chief-a' } }, + bots: { + 'chief-a': { id: 'chief-a', sessionId: 'chief-a', name: 'Chief A', instructions: '', createdAt: 1, ownerId: 'owner-1' }, + 'chief-b': { id: 'chief-b', sessionId: 'chief-b', name: 'Chief B', instructions: '', createdAt: 2, ownerId: 'owner-1' }, + 'spec-a': { id: 'spec-a', sessionId: 'spec-a', name: 'Writer A', instructions: '', createdAt: 3, ownerId: 'owner-1' }, + 'spec-b': { id: 'spec-b', sessionId: 'spec-b', name: 'Writer B', instructions: '', createdAt: 4, ownerId: 'owner-1' }, + }, + rooms: { + war: { + id: 'war', + name: 'War room', + members: ['chief-a', 'chief-b'], + admin: 'chief-a', + named: true, + log: [], + seq: 0, + seen: {}, + participants: [ + { kind: 'human', id: 'owner-1', role: 'admin', displayName: 'Alice', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'chief-a', role: 'admin', displayName: 'Chief A', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'chief-b', role: 'member', displayName: 'Chief B', joinedSeq: 0, joinedAt: 1 }, + ], + }, + }, + exchanges: [], + exchangeTurns: {}, + questions: {}, +} + +const CODER = { handle: '@coder.bob.home', name: 'Coder', home: '@bob.home', homeName: 'Bob', owner: 'Bob', plugin: 'social', remote: true } + +function host(t, saved = TEAM) { + const dir = mkdtempSync(join(tmpdir(), 'ds-bot-room-line-')) + writeFileSync(join(dir, 'state.json'), typeof saved === 'string' ? saved : JSON.stringify(saved)) + const warnings = [] + const disposers = [] + const ctx = { + logger: { warn: (...args) => warnings.push(args.join(' ')) }, + effect(execute) { + const dispose = execute() + if (typeof dispose === 'function') disposers.push(dispose) + }, + on: () => () => {}, + tools: { register: () => {} }, + systemPrompt: { section: () => () => {} }, + sessions: {}, + get: () => undefined, + set: () => {}, + } + const sent = [] + const rt = { + ctx, + home: dir, + statePath: join(dir, 'state.json'), + autoRoomName: () => 'Room', + retitleRoom: async () => {}, + dropGroupSession: () => {}, + warn: (...args) => warnings.push(args.join(' ')), + flybotSend: async (msg) => { sent.push(msg); return { action: 'sent' } }, + } + installStore(rt) + installMemberAuth(rt) + installAuthz(rt) + installRooms(rt) + installReach(rt) + installRoster(rt) + installGroups(rt) + t.after(async () => { + await Promise.all(disposers.splice(0).map(cleanup => cleanup())) + rmSync(dir, { recursive: true, force: true, maxRetries: 5 }) + }) + return { rt, sent, dir, warnings } +} + +const publish = (rt, list = [CODER]) => { rt.state.flybot ??= {}; rt.state.flybot.remote = { social: list } } + +async function grantToFriend(rt, botId, grantee = '@bob.home') { + const current = rt.grantsOf?.(botId) ?? [] + if (current.some(g => g.grantee === grantee)) return + await rt.setGrants(botId, [...current, { grantee }]) +} + +// A room of this home with one seat on another home, which is the surface the leak crossed. +async function withRemoteSeat(rt) { + const room = rt.state.rooms.war + room.participants = room.participants.filter(p => p.id !== 'chief-b') + room.members = room.members.filter(id => id !== 'chief-b') + publish(rt) + await grantToFriend(rt, 'chief-a') + await grantToFriend(rt, 'chief-b') + const invite = await rt.createInvitation({ createdByMemberId: 'owner-1' }) + const alice = await rt.redeemInvitation(invite.token, { displayName: 'Alice' }) + const actor = await rt.authenticate(alice.sessionToken) + await rt.addRoomParticipant('war', { kind: 'bot', remote: true, handle: '@coder.bob.home' }, actor) + return { room, actor } +} + +// The wire: the envelope the other home decodes. +const wire = (sent) => { + const last = sent.at(-1) + return last === undefined ? undefined : { to: last.to, ...parseRoomShare(last.text) } +} + +// A room line may name a person, a Bot and an attachment; it may not name this host's folders and it +// may not carry a credential — not even one that only looks like one (#2699). +const localDetail = [ + [/(?:^|[\s("'`=:,[)])\.?\/[A-Za-z]/u, 'an absolute path'], + [/ghp_[A-Za-z0-9]+/, 'a token-shaped string'], + [/token=/i, 'a token assignment'], +] + +function assertNoLocalDetail(text, dir, label = 'the room line') { + for (const [pattern, what] of localDetail) { + assert.doesNotMatch(String(text), pattern, `${label} must not carry ${what}: ${text}`) + } + if (dir !== undefined) assert.equal(String(text).includes(dir), false, `${label} names this home's folder: ${text}`) +} + +test('#2699: an unread image in a group names the attachment and its size, never this host\'s path', async (t) => { + const { rt, sent, dir } = host(t) + await rt.load() + await withRemoteSeat(rt) + + rt.config = { textOnlyModels: [] } + rt.catalog = async () => [] + rt.defaultRef = () => undefined + rt.ownRequest = () => false + installImages(rt) + + // The attachment store's layout, a real file so `attachmentFilePath` would answer a path. + const id = `sha256:${'ab'.repeat(32)}` + const objectPath = attachmentObjectPath(rt.home, id) + mkdirSync(join(objectPath, '..'), { recursive: true }) + writeFileSync(objectPath, 'fake-bytes') + const attachment = { attachmentId: id, name: 'holiday.png', mediaType: 'image/png', bytes: 9 } + + const [text] = await rt.imagesForGroup([attachment], 'war') + assert.match(text, /\[Image "holiday\.png" \(image\/png, 9 bytes\): it could not be read right now\./, 'the room still says which attachment and how big it is') + assert.equal(text.includes(dir), false, `the group placeholder names this home: ${text}`) + assertNoLocalDetail(text, dir, 'the group placeholder') + // The placeholder alone never names a path, even when one is offered: only the local reader may. + assertNoLocalDetail(imagePlaceholder(attachment, { path: objectPath, group: true }), dir, 'the group form with a path') + + sent.length = 0 + await rt.fanoutRoomLine(rt.state.rooms.war, { seq: 1, who: 'Alice', text, authorKind: 'human', authorId: 'owner-1' }) + const out = wire(sent) + assert.equal(out?.to, '@coder.bob.home', 'the line still reaches the far side') + assertNoLocalDetail(out.text, dir, "the friend's Bot") + assert.match(out.text, /holiday\.png/) + + // The reader that is this host may still be told where the bytes are (#779): a Bot chat keeps it. + const local = imagePlaceholder(attachment, { path: objectPath }) + assert.ok(local.includes(`"${objectPath}"`), local) +}) + +test('#2699: a crashed external Bot reports how it ended, and keeps its stderr in this host\'s log', async (t) => { + const { rt, sent, dir, warnings } = host(t) + await rt.load() + await withRemoteSeat(rt) + + const lines = [] + const realExternalRoomLine = rt.externalRoomLine + rt.externalRoomLine = async (room, line) => { + const stored = await realExternalRoomLine(room, line) + lines.push(stored) + return stored + } + + const room = rt.state.rooms.war + // A child that dies at startup: it names a file it could not open and prints a token-shaped string. + const CRASH = `process.stderr.write(${JSON.stringify(`Error: ENOENT: no such file or directory, open '${HOST_PATH}' token=${TOKEN}\n`)});setTimeout(() => process.exit(1), 10)` + const member = { id: 'chief-a', name: 'Chief A', external: { driver: 'subprocess', command: [process.execPath, '-e', CRASH], cwd: rt.home } } + const participant = createExternalRoomParticipant(rt) + t.after(() => participant.close()) + await participant.open({ room, member }) + + for (let i = 0; i < 60 && lines.length === 0; i += 1) await new Promise(r => setTimeout(r, 50)) + assert.equal(lines.length, 1, 'one crash line for the room') + const line = lines[0].text + assert.match(line, /crashed/, line) + assert.match(line, /\(exit 1\)/, `the room reads how the process ended: ${line}`) + assertNoLocalDetail(line, dir, 'the crash line') + + const out = wire(sent) + assert.equal(out?.to, '@coder.bob.home') + assertNoLocalDetail(out.text, dir, "the friend's Bot") + assert.match(out.text, /crashed/) + + // The diagnosis stays where the owner can read it and the far side cannot. + const logged = warnings.join('\n') + assert.ok(logged.includes(HOST_PATH), `the local log keeps the crash detail: ${logged}`) + assert.ok(logged.includes(TOKEN), 'the local log keeps what the child printed') +}) + +test('#2699: a process that cannot start says so to this host, not the room', async (t) => { + const { rt, sent, dir, warnings } = host(t) + await rt.load() + await withRemoteSeat(rt) + + const lines = [] + const realExternalRoomLine = rt.externalRoomLine + rt.externalRoomLine = async (room, line) => { + const stored = await realExternalRoomLine(room, line) + lines.push(stored) + return stored + } + + const room = rt.state.rooms.war + const command = join(dir, 'gone', 'agent-2699') + const member = { id: 'chief-a', name: 'Chief A', external: { driver: 'subprocess', command: [command] } } + const participant = createExternalRoomParticipant(rt) + t.after(() => participant.close()) + await participant.open({ room, member }) + + for (let i = 0; i < 60 && lines.length === 0; i += 1) await new Promise(r => setTimeout(r, 50)) + assert.equal(lines.length, 1, 'the room hears that it crashed') + assertNoLocalDetail(lines[0].text, dir, 'the crash line') + assert.equal(wire(sent)?.to, '@coder.bob.home') + assertNoLocalDetail(wire(sent).text, dir, "the friend's Bot") + assert.ok(warnings.join('\n').includes(command), 'this host\'s log names the path that could not be started') +}) + +// The one seam every stored room line passes, whichever writer made it (#1251): whatever the host +// redacts must not survive into the log or the wire. `rt.redactSecrets` is what `secrets.js` installs +// (`redactSecrets: text => redact(text)`); this fixture answers with the same shape. +test('#2699: every stored room line goes through the host redactor, whichever writer made it', async (t) => { + const { rt, sent } = host(t) + await rt.load() + const { room, actor } = await withRemoteSeat(rt) + rt.redactSecrets = text => String(text).replace(/ghp_[A-Za-z0-9]+/g, '[secret:GITHUB_TOKEN]') + + sent.length = 0 + await rt.postRoomMessage('war', { text: `the deploy key is ${TOKEN}, keep it out of the room` }, actor) + + const stored = (room.log ?? []).at(-1) + assert.match(stored.text, /\[secret:GITHUB_TOKEN\]/, `the stored line is redacted: ${stored.text}`) + assert.equal(stored.text.includes(TOKEN), false, stored.text) + const out = wire(sent) + assert.equal(out?.to, '@coder.bob.home') + assert.match(out.text, /\[secret:GITHUB_TOKEN\]/, out.text) + assert.equal(out.text.includes(TOKEN), false, "the friend's Bot never reads the value") +})