diff --git a/changelog.d/2167-diagnostics-readonly-reason.fixed.md b/changelog.d/2167-diagnostics-readonly-reason.fixed.md new file mode 100644 index 000000000..f202f453d --- /dev/null +++ b/changelog.d/2167-diagnostics-readonly-reason.fixed.md @@ -0,0 +1 @@ +The `Read-only` row of a host diagnostics report no longer says another dsh holds the team when it does not: a team file saved by a newer FlyBot reads `update FlyBot` and an unreadable one reads `could not be read` with its cause, exactly the sentence the sidebar shows, while a window that really lost the team to a live `state.lock` keeps the other-dsh wording ([#2167](https://github.com/ThinkFlowLab/FlyBot/issues/2167)). diff --git a/core/src/host/diagnostics.js b/core/src/host/diagnostics.js index 013cc9b17..19558217b 100644 --- a/core/src/host/diagnostics.js +++ b/core/src/host/diagnostics.js @@ -233,7 +233,10 @@ export function install(rt) { node: process.version, team: `${count(bots.length, 'Bot', 'Bots')}, ${rt.state.mainBotIds.length} Main, ${count(groups, 'group chat', 'group chats')}`, models: models.length > 0 ? models.join(', ') : undefined, - readOnly: readOnly ? 'yes, another dsh holds the team' : undefined, + // The store's own sentence (#2153): `stuck` (a team file that could not be read) and a + // live state.lock are different faults, and the sidebar shows this same sentence, so the + // report must not compress both into "another dsh holds the team". + readOnly: readOnly ? clean(cut(readOnly, 200).text) : undefined, // The writer is opened before the lock is asked for, so in a window another dsh keeps // read-only `save()` returns before `journal.record()` and no line is written at all; // claiming `recording` there promises a line the file will not get. A journal that diff --git a/core/test/diagnostics.test.js b/core/test/diagnostics.test.js index 9561654d1..e2d0d0021 100644 --- a/core/test/diagnostics.test.js +++ b/core/test/diagnostics.test.js @@ -2,11 +2,12 @@ import assert from 'node:assert/strict' import { linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import fsp from 'node:fs/promises' import { syncBuiltinESMExports } from 'node:module' -import { homedir, tmpdir } from 'node:os' +import { homedir, hostname, tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' import { ISSUES_URL, cleanText, install as installDiagnostics, reportMarkdown } from '../src/host/diagnostics.js' import { openJournal, stateText } from '../src/host/journal.js' +import { install as installStore } from '../src/host/store.js' import { failureLine, install as installTurns, turnFailure } from '../src/host/turns.js' import { errorCode } from '../src/host/channel.js' import { SOURCE_KIND } from '../src/host/constants.js' @@ -309,7 +310,7 @@ test('a read-only window does not say the state journal is recording (#2153)', a const readOnly = () => 'Another dsh (pid 1 on host) holds the team' const journal = openJournal({ path: join(home, 'journal', 'state.jsonl'), warn: () => {} }) const held = await host(home, { journal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) - assert.match(held.markdown, /^\| Read-only \| yes, another dsh holds the team \|$/m) + assert.match(held.markdown, /^\| Read-only \| Another dsh \(pid 1 on host\) holds the team \|$/m) assert.doesNotMatch(held.markdown, /state journal: recording/) assert.match(held.markdown, /^state journal: paused \(this window is read-only\)$/m) assert.equal(held.facts.stateJournal, 'paused (this window is read-only)') @@ -326,6 +327,70 @@ test('a read-only window does not say the state journal is recording (#2153)', a assert.match(stoppedHeld.markdown, /^state journal: stopped \(.*not a plain file.*\)$/m) }) +// The Read-only row answered one constant for every window that may not write, but the store +// answers three ways (#2153): `stuck` (the team file cannot be read) is not a live lock at all, +// and a report that calls a newer or unreadable team file "another dsh holds the team" sends the +// reader to close a host that is not running. These four build the real store and the real +// diagnostics on one runtime, so the sentence is the one the host actually produces. +async function realHost(t, { saved, locked = false } = {}) { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-ro-')) + const statePath = join(home, 'state.json') + if (saved === 'dir') mkdirSync(statePath) + else if (saved !== undefined) writeFileSync(statePath, JSON.stringify(saved)) + // A lock file naming a live process is exactly what a second dsh leaves behind. + if (locked) writeFileSync(join(home, 'state.lock'), JSON.stringify({ pid: 1, host: hostname(), token: 'the-other-dsh' })) + const disposers = [] + const rt = { + ctx: { + logger: { warn: () => {} }, + effect(execute) { + const dispose = execute() + if (typeof dispose === 'function') disposers.push(dispose) + }, + }, + home, + statePath, + config: { diagnosticsFile: false }, + autoRoomName: () => 'Room', + retitleRoom: async () => {}, + warn: () => {}, + } + installStore(rt) + await rt.load() + installDiagnostics(rt) + t.after(async () => { + await Promise.all(disposers.splice(0).map(dispose => dispose())) + rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + }) + return { home, readOnly: rt.readOnly(), report: await rt.diagnostics.report({}) } +} + +test('the Read-only row says a newer team file needs an update, not that another dsh holds the team (#2167)', async (t) => { + const { home, readOnly, report } = await realHost(t, { saved: { version: 99 } }) + assert.match(readOnly, /update FlyBot/) + assert.ok(report.markdown.includes(`| Read-only | The Bot team file ${join(home, 'state.json')} could not be read (saved by a newer FlyBot; update FlyBot). FlyBot does not change it; fix or move the file and reload. |`), report.markdown) + assert.doesNotMatch(report.markdown, /another dsh holds the team/) +}) + +test('the Read-only row says an unreadable team file could not be read, not that another dsh holds the team (#2167)', async (t) => { + const { report } = await realHost(t, { saved: 'dir' }) + assert.match(report.markdown, /^\| Read-only \| The Bot team file .* could not be read \(EISDIR\)\. FlyBot does not change it; fix or move the file and reload\. \|$/m) + assert.doesNotMatch(report.markdown, /another dsh holds the team/) +}) + +test('the Read-only row keeps the other-dsh wording when a live lock names it (#2167)', async (t) => { + const { report } = await realHost(t, { locked: true }) + assert.ok(report.markdown.includes('open in another dsh (Web or Desktop, pid 1 on '), report.markdown) + assert.match(report.markdown, /This window shows the team but cannot change it; close the other one and reload\./) + assert.doesNotMatch(report.markdown, /could not be read/) +}) + +test('a writable window has no Read-only row', async (t) => { + const { report } = await realHost(t) + assert.equal(report.facts.readOnly, undefined) + assert.doesNotMatch(report.markdown, /^\| Read-only \|/m) +}) + test('reportMarkdown says when nothing went wrong', () => { const text = reportMarkdown({ facts: { botVersion: '0.1.0', os: 'linux' } }) assert.match(text, /No host errors recorded\./) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 7c2ca6630..3e9aab6df 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -405,6 +405,7 @@ Work a member hands a Bot with `message_bot` from its group Session runs in a hi ### Diagnostics - Host diagnostics never depend on `ctx.logger`: DSH may export no logger, and a test or plugin-local host may carry one without `warn`. `rt.warn` still writes stderr and the diagnostics log in that case (`src/host/diagnostics.js`). Plugins reach the same log through `flybot.warn` (#68); see [plugin-runtime](plugin-runtime.md). +- The `Read-only` row of a report repeats this host's own answer rather than one constant: `rt.readOnly()` is `undefined` while the window writes, a sentence about a live `state.lock` when another dsh holds the team, and a sentence about the team file when it is `stuck` (a newer version, a file that will not parse, an unreadable path). The report shows that sentence — `clean(cut(answer, 200))`, so saved keys, the home folder and `|` are handled like any other report text — the same one the sidebar shows, so a newer team file reads `update FlyBot` and an unreadable one reads `could not be read`, never `another dsh holds the team`; a writable window still adds no row ([#2167](https://github.com/ThinkFlowLab/FlyBot/issues/2167)). - The log itself is one leaf file under the home, and a write asks it before it appends ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `logs/diagnostics.jsonl` is a link, a hard link or anything other than a plain file of its own — the line is dropped with one line on stderr that carries no home folder, the entries stay readable in memory, and `clear` leaves a file behind such a link untouched rather than truncating it. ### Untrusted fences in every shared room diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 3636360c1..26b1b4d7d 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -4884,12 +4884,15 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js return { botVersion: ownVersion(), dshVersion: dshVersion(), -@@ -199,12 +199,19 @@ export function install(rt) { +@@ -199,12 +199,22 @@ export function install(rt) { node: process.version, team: `${count(bots.length, 'Bot', 'Bots')}, ${rt.state.mainBotIds.length} Main, ${count(groups, 'group chat', 'group chats')}`, models: models.length > 0 ? models.join(', ') : undefined, - readOnly: rt.readOnly?.() ? 'yes, another dsh holds the team' : undefined, -+ readOnly: readOnly ? 'yes, another dsh holds the team' : undefined, ++ // The store's own sentence (#2153): `stuck` (a team file that could not be read) and a ++ // live state.lock are different faults, and the sidebar shows this same sentence, so the ++ // report must not compress both into "another dsh holds the team". ++ readOnly: readOnly ? clean(cut(readOnly, 200).text) : undefined, + // The writer is opened before the lock is asked for, so in a window another dsh keeps + // read-only `save()` returns before `journal.record()` and no line is written at all; + // claiming `recording` there promises a line the file will not get. A journal that diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index 924848f2a..ccf18aebb 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -795,18 +795,20 @@ diff --git a/scripts/lint.mjs b/scripts/lint.mjs diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js --- a/test/diagnostics.test.js +++ b/test/diagnostics.test.js -@@ -1,11 +1,12 @@ +@@ -1,11 +1,13 @@ import assert from 'node:assert/strict' -import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import fsp from 'node:fs/promises' import { syncBuiltinESMExports } from 'node:module' - import { homedir, tmpdir } from 'node:os' +-import { homedir, tmpdir } from 'node:os' ++import { homedir, hostname, tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' -import { cleanText, install as installDiagnostics, reportMarkdown } from '../src/host/diagnostics.js' +import { ISSUES_URL, cleanText, install as installDiagnostics, reportMarkdown } from '../src/host/diagnostics.js' +import { openJournal, stateText } from '../src/host/journal.js' ++import { install as installStore } from '../src/host/store.js' import { failureLine, install as installTurns, turnFailure } from '../src/host/turns.js' import { errorCode } from '../src/host/channel.js' import { SOURCE_KIND } from '../src/host/constants.js' @@ -1003,7 +1005,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js test('diagnostics append fallback redacts an error message', async (t) => { const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) -@@ -133,6 +133,58 @@ test('diagnostics append fallback redacts an error message', async (t) => { +@@ -133,6 +133,122 @@ test('diagnostics append fallback redacts an error message', async (t) => { assert.match(stderr.join('\n'), /api_key=\[hidden\]/) }) @@ -1042,7 +1044,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js + const readOnly = () => 'Another dsh (pid 1 on host) holds the team' + const journal = openJournal({ path: join(home, 'journal', 'state.jsonl'), warn: () => {} }) + const held = await host(home, { journal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) -+ assert.match(held.markdown, /^\| Read-only \| yes, another dsh holds the team \|$/m) ++ assert.match(held.markdown, /^\| Read-only \| Another dsh \(pid 1 on host\) holds the team \|$/m) + assert.doesNotMatch(held.markdown, /state journal: recording/) + assert.match(held.markdown, /^state journal: paused \(this window is read-only\)$/m) + assert.equal(held.facts.stateJournal, 'paused (this window is read-only)') @@ -1058,6 +1060,70 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js + const stoppedHeld = await host(home, { journal: stoppedJournal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) + assert.match(stoppedHeld.markdown, /^state journal: stopped \(.*not a plain file.*\)$/m) +}) ++ ++// The Read-only row answered one constant for every window that may not write, but the store ++// answers three ways (#2153): `stuck` (the team file cannot be read) is not a live lock at all, ++// and a report that calls a newer or unreadable team file "another dsh holds the team" sends the ++// reader to close a host that is not running. These four build the real store and the real ++// diagnostics on one runtime, so the sentence is the one the host actually produces. ++async function realHost(t, { saved, locked = false } = {}) { ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-ro-')) ++ const statePath = join(home, 'state.json') ++ if (saved === 'dir') mkdirSync(statePath) ++ else if (saved !== undefined) writeFileSync(statePath, JSON.stringify(saved)) ++ // A lock file naming a live process is exactly what a second dsh leaves behind. ++ if (locked) writeFileSync(join(home, 'state.lock'), JSON.stringify({ pid: 1, host: hostname(), token: 'the-other-dsh' })) ++ const disposers = [] ++ const rt = { ++ ctx: { ++ logger: { warn: () => {} }, ++ effect(execute) { ++ const dispose = execute() ++ if (typeof dispose === 'function') disposers.push(dispose) ++ }, ++ }, ++ home, ++ statePath, ++ config: { diagnosticsFile: false }, ++ autoRoomName: () => 'Room', ++ retitleRoom: async () => {}, ++ warn: () => {}, ++ } ++ installStore(rt) ++ await rt.load() ++ installDiagnostics(rt) ++ t.after(async () => { ++ await Promise.all(disposers.splice(0).map(dispose => dispose())) ++ rmSync(home, { recursive: true, force: true, maxRetries: 5 }) ++ }) ++ return { home, readOnly: rt.readOnly(), report: await rt.diagnostics.report({}) } ++} ++ ++test('the Read-only row says a newer team file needs an update, not that another dsh holds the team (#2167)', async (t) => { ++ const { home, readOnly, report } = await realHost(t, { saved: { version: 99 } }) ++ assert.match(readOnly, /update FlyBot/) ++ assert.ok(report.markdown.includes(`| Read-only | The Bot team file ${join(home, 'state.json')} could not be read (saved by a newer FlyBot; update FlyBot). FlyBot does not change it; fix or move the file and reload. |`), report.markdown) ++ assert.doesNotMatch(report.markdown, /another dsh holds the team/) ++}) ++ ++test('the Read-only row says an unreadable team file could not be read, not that another dsh holds the team (#2167)', async (t) => { ++ const { report } = await realHost(t, { saved: 'dir' }) ++ assert.match(report.markdown, /^\| Read-only \| The Bot team file .* could not be read \(EISDIR\)\. FlyBot does not change it; fix or move the file and reload\. \|$/m) ++ assert.doesNotMatch(report.markdown, /another dsh holds the team/) ++}) ++ ++test('the Read-only row keeps the other-dsh wording when a live lock names it (#2167)', async (t) => { ++ const { report } = await realHost(t, { locked: true }) ++ assert.ok(report.markdown.includes('open in another dsh (Web or Desktop, pid 1 on '), report.markdown) ++ assert.match(report.markdown, /This window shows the team but cannot change it; close the other one and reload\./) ++ assert.doesNotMatch(report.markdown, /could not be read/) ++}) ++ ++test('a writable window has no Read-only row', async (t) => { ++ const { report } = await realHost(t) ++ assert.equal(report.facts.readOnly, undefined) ++ assert.doesNotMatch(report.markdown, /^\| Read-only \|/m) ++}) + test('reportMarkdown says when nothing went wrong', () => { const text = reportMarkdown({ facts: { botVersion: '0.1.0', os: 'linux' } })