From a4f3735c7dd9fb787ee84fc9150a1f663dc03232 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 7 Nov 2026 22:35:12 +0800 Subject: [PATCH] fix(host): refuse a workflow folder that cannot be listed A `workflows/` folder that is a real directory but cannot be read listed nothing, while `open` still read a definition by name, so a save answered `ok: true` with an empty `own`. The listing half now refuses with `workflow/unreadable`, a landed write keeps `wrote: true`, and opening by name still works. Refs #2614 --- changelog.d/2614.fixed.md | 1 + core/src/host/workflow-store.js | 36 +++++--- core/test/workflow-store.test.js | 83 ++++++++++++++++++- docs/subsystems/core-host.md | 2 +- .../ds-bot/files/src/host/workflow-store.js | 36 +++++--- .../ds-bot/files/test/workflow-store.test.js | 83 ++++++++++++++++++- 6 files changed, 218 insertions(+), 23 deletions(-) create mode 100644 changelog.d/2614.fixed.md diff --git a/changelog.d/2614.fixed.md b/changelog.d/2614.fixed.md new file mode 100644 index 000000000..f3f5775fb --- /dev/null +++ b/changelog.d/2614.fixed.md @@ -0,0 +1 @@ +A workspace's `workflows/` folder that is there but cannot be listed no longer answers "no workflows": the host now says `workflow/unreadable` and names the folder, a save that landed still reports `wrote: true`, and opening a workflow by name keeps working ([#2614](https://github.com/ThinkFlowLab/FlyBot/issues/2614)). diff --git a/core/src/host/workflow-store.js b/core/src/host/workflow-store.js index cfc995a79..dca74aa0d 100644 --- a/core/src/host/workflow-store.js +++ b/core/src/host/workflow-store.js @@ -75,14 +75,13 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, const refuseNotFile = (file) => ({ ok: false, code: 'workflow/not-a-file', message: `the workflow file ${file} is not a plain file of its own, so nothing is written there`, problems: [] }) // A directory may not exist yet (a workspace nobody has customized anything in), and a file in it // may be someone's hand-edit that does not parse: one broken file must not take the list down. + // A real folder that cannot be listed is not that case (#2614): `list` turns the failure into a + // refusal, because an empty list here would sit beside the files `open` still reads by name. const ownFiles = () => { if (!folderReal()) return [] - try { - return readdirSync(dir).filter((name) => name.endsWith('.json') && name !== PROJECT_FILE).sort() - } catch { - return [] - } + return readdirSync(dir).filter((name) => name.endsWith('.json') && name !== PROJECT_FILE).sort() } + const refuseUnreadable = () => ({ ok: false, code: 'workflow/unreadable', message: `the workflow folder ${dir} is there but cannot be listed, so its workflows cannot be read`, problems: [] }) const readOwn = (id) => { if (!folderReal()) return null @@ -116,7 +115,13 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, } const list = () => { - const own = ownFiles().flatMap((name) => { + let names + try { + names = ownFiles() + } catch { + return refuseUnreadable() + } + const own = names.flatMap((name) => { const definition = readOwn(name.slice(0, -'.json'.length)) if (definition === null || definition.id === '') { skipped(join(dir, name), name, dir) @@ -141,9 +146,13 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, const { definition, source } = found const target = typeof node === 'string' && node !== '' ? node : definition.nodes.find((entry) => entry.column !== false)?.id ?? definition.nodes[0]?.id ?? null const answer = factsFor({ id: definition.id, node: target }) + const listed = list() return { ok: true, - ...list(), + // A folder that cannot be listed still opens a definition by name (reading one name needs + // only `+x`), so the refusal belongs to the listing half: the listing fields are left out + // rather than spreading an empty `own` over this answer (#2614). + ...(listed.ok ? listed : {}), workflow: { id: definition.id, name: definition.name, @@ -185,11 +194,16 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, return null } })() - if (previous !== bytes) { + const wrote = previous !== bytes + if (wrote) { mkdirSync(dir, { recursive: true }) writeFileSync(path, bytes, { mode: 0o600 }) } - return { ok: true, id: def.id, bytes, changed: previous !== bytes, ...list() } + const listed = list() + // A landed write and a folder that refuses to be listed are both true: saying only `ok: false` + // would hide the write, and `ok: true` beside an empty `own` would hide the folder (#2614). + if (!listed.ok) return { ...listed, id: def.id, wrote, changed: wrote } + return { ok: true, id: def.id, bytes, changed: wrote, ...listed } } const customize = ({ from, id, name } = {}) => { @@ -209,7 +223,9 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, if (!fileWritable(path)) return refuseNotFile(path) mkdirSync(dir, { recursive: true }) writeFileSync(path, `${JSON.stringify({ default: found.definition.id }, null, 2)}\n`, { mode: 0o600 }) - return { ok: true, ...list() } + const listed = list() + if (!listed.ok) return { ...listed, wrote: true, changed: true } + return { ok: true, ...listed } } return { diff --git a/core/test/workflow-store.test.js b/core/test/workflow-store.test.js index a40eaf148..a6596ac92 100644 --- a/core/test/workflow-store.test.js +++ b/core/test/workflow-store.test.js @@ -3,7 +3,7 @@ // actor (acceptance 6 and 7). import assert from 'node:assert/strict' import { createHash } from 'node:crypto' -import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -521,3 +521,84 @@ test('#2609: a stored default that leaves the folder is answered as none', (t) = assert.equal(listed.default, null, 'a stored key is a file name only when it is one') assert.deepEqual(listed.own, []) }) + +// --------------------------------------------------------------------------- +// #2614: a workflow folder that is there but cannot be listed (a restore with the owner bits +// wrong, or a `chmod -R` accident) answered `ok: true` with an empty `own`, while `open` still +// read a definition by name: listing a folder needs `+r`, reading a name in it only `+x`, so the +// two halves disagreed. The listing half refuses with `workflow/unreadable`, a write that landed +// still says `wrote: true`, and getting the permission back lists the files again. +// Root ignores the permission bits, so both cases are skipped there. + +const unlistableFolder = (t) => { + const root = mkdtempSync(join(tmpdir(), 'flybot-workflows-')) + const dir = join(root, 'workflows') + mkdirSync(dir, { recursive: true }) + writeFileSync(join(dir, 'mine.json'), JSON.stringify({ ...blankWorkflow, id: 'mine', name: 'Mine' })) + t.after(() => { + chmodSync(dir, 0o755) + rmSync(root, { recursive: true, force: true }) + }) + chmodSync(dir, 0o311) + return dir +} + +test('#2614: a workflow folder that cannot be listed refuses the list instead of answering empty', (t) => { + if (process.getuid?.() === 0) { + t.skip('root ignores the folder permission bits') + return + } + const dir = unlistableFolder(t) + const store = createWorkflowStore({ dir }) + const listed = store.list() + assert.equal(listed.ok, false) + assert.equal(listed.code, 'workflow/unreadable') + assert.ok(listed.message.includes(`the workflow folder ${dir} is there but cannot be listed`), listed.message) + assert.equal('own' in listed, false, 'a refusal never carries an empty own list') + // Reading a name out of the folder needs only `+x`, so it still answers: that is how the two + // halves disagreed before the refusal existed. + const opened = store.open('mine') + assert.equal(opened.ok, true) + assert.equal(opened.workflow.source, 'workspace') + assert.equal(opened.workflow.definition.name, 'Mine') + // Put the permission back: the same store lists the file again. + chmodSync(dir, 0o755) + const back = store.list() + assert.equal(back.ok, true) + assert.deepEqual(back.own.map((entry) => entry.id), ['mine']) +}) + +test('#2614: a save that landed in an unlistable folder says wrote: true and refuses the list', (t) => { + if (process.getuid?.() === 0) { + t.skip('root ignores the folder permission bits') + return + } + const dir = unlistableFolder(t) + const store = createWorkflowStore({ dir }) + const saved = store.save({ definition: { ...blankWorkflow, id: 'mine', name: 'Mine 2' } }) + assert.equal(saved.ok, false) + assert.equal(saved.code, 'workflow/unreadable') + assert.equal(saved.wrote, true) + assert.equal(saved.changed, true) + assert.equal('own' in saved, false, 'a refusal never carries an empty own list') + assert.equal(JSON.parse(readFileSync(join(dir, 'mine.json'), 'utf8')).name, 'Mine 2', 'the write landed') + // An unedited save writes nothing, and still refuses the list it could not read. + const again = store.save({ definition: store.open('mine').workflow.definition }) + assert.equal(again.ok, false) + assert.equal(again.code, 'workflow/unreadable') + assert.equal(again.wrote, false) + assert.equal(again.changed, false) + // A default lands too, and says the same thing about the listing half. + const set = store.setDefault({ id: 'mine' }) + assert.equal(set.ok, false) + assert.equal(set.code, 'workflow/unreadable') + assert.equal(set.wrote, true) + assert.equal(JSON.parse(readFileSync(join(dir, 'project.json'), 'utf8')).default, 'mine', 'the default landed') + // Reading the default back by name needs only `+x`; the listing half is what refused. + assert.equal(store.open('mine').ok, true) + chmodSync(dir, 0o755) + const back = store.list() + assert.equal(back.ok, true) + assert.equal(back.default, 'mine') + assert.deepEqual(back.own.map((entry) => entry.id), ['mine']) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index cda3c3139..e032144b9 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -67,7 +67,7 @@ The server side of the patched `ds-bot` core (`core/`): the part of [ds-bot-core | `core/src/shared/workflow-doc.js` | The definition as a file (#1272 step 3): `workflowBytes(def)` is canonical — fixed key order, two spaces, a trailing newline, a field a definition does not state left out — so a load → save round trip is byte-identical, a definition diffs in git, and `column: false` is written while an absent column is not (they are two different decisions, #1265). `isWorkflowId` is the file-name rule (lowercase, digits, hyphens) and `PRESET_IDS` names the two ids a workspace file may not claim; `copyOfPreset(preset, { id, name })` is what *Customize* writes, and `differingFields(a, b)` is how the copy is held to "differs only in id/name" (acceptance 6). No I/O. Tests: `core/test/workflow-doc.test.js`. | | `core/src/shared/workflow-preview.js` | The guard preview the editor shows (#1272 step 3, acceptance 5): `previewNode(def, nodeId, answer)` asks `workflow.js`'s `guardRows` for one row per guard in the definition's order — `holds` (read and passing, no sentence), `refuse` (the producer's own sentence) or `unreadable` — and when the reader answered nothing (`{ ok: false, why }`) every row says it could not read the fact, with the reason named and no pass anywhere. It reads the same function `enterNode` reads, so the preview and a card's refusal cannot disagree. Tests: `core/test/workflow-preview.test.js`. | | `core/src/shared/workflow-run.js` | The running layer's pure half (#1316, #1272 step 4). `planJob({ definition, job, cards, facts, hold })` answers which node cards a job may create now: every decision is `enterNode`'s, so a node whose input is missing, stale or unreadable opens nothing and its refusal is that function's own sentence byte for byte, and a node that already has a card is in neither list — a card parented to another job is not this job's card, so `cardsOfJob` is what makes `(job, node)` one card across a replayed tick or a restarted host, even when a caller hands in the whole board. An open entry is the card the board already places (`parentId` the job card, `workflow`, `node`) plus the fields the pull queue reads (`role`, `queueRole`, `assignee: null`), so the hand-off is the claim and never an `@`. A held job opens nothing on any node, in the definition's own sentence: the planner renders that definition's `no-hold` guard (`pr` equal to `none`, refused for `hold`) through `renderRefusal`, which is the merge gate's wording byte for byte, and a definition guarding no held pull request gets `refusal: null` and the holder's name instead of a sentence this layer wrote. `jobDone` answers pin 6 — a job is done when its terminal node's outputs are present and current, never because a card moved — and `completions` is step (c)'s pure half: a done card whose declared outputs are current, append-only, an entry already in `history` never returned again. No fs, no clock, no timer, no network, no host import: `task-drive.js` `driveOnce` stays the only beat. Tests: `core/test/workflow-run.test.js`. | -| `core/src/host/workflow-store.js` | Where a definition lives (#1272 step 3): the presets read from their modules plus the owner's `workflows/.json` in the workspace, and the only writer. `save` validates with the engine's own `validateWorkflow` (the roster's roles are the team; an empty roster is no team to check), refuses a preset id (`workflow/preset-read-only` — *Customize* is the only way to start from one), writes `workflow-doc.js`'s bytes only when they changed, and answers `changed: false` for an unedited definition (acceptance 7). `customize`, `setDefault` (the project's `workflows/project.json`), `list`, `open` (definition + bytes + layout + preview) and `definitions()` (the presets plus the workspace files) round it out; `install` wires `rt.workflowEndpoint` for `workflows` (read) and `workflow-save` (write, in `authz.js`'s `TASK_ENDPOINTS`), so the client writes a file through the host and never to GitHub. A file that does not parse is skipped with one warning instead of breaking the list. The `workflows` folder itself must be a real directory too ([#2057](https://github.com/ThinkFlowLab/FlyBot/issues/2057), [#2068](https://github.com/ThinkFlowLab/FlyBot/issues/2068)): `dirKindSync` from `src/host/real-dir.js` is asked before every listing and read, so a link, a plain file or a check that cannot run lists nothing and opens nothing — `own` is empty, `default` is `null`, `open` answers `workflow/not-found`, `definitions()` answers the presets — which is exactly the answer a missing folder gives, and the linked folder is named once per folder and process in one warning (`ds-bot: workflow folder is not a plain directory …; keeping it empty`). The write side follows the same rule: `save`, `customize` and `setDefault` still create a missing folder as before and refuse a linked one instead of writing into it (`workflow/not-a-folder`), and a workflow file that does not parse is reported once per file and process. The file itself is asked as well ([#2079](https://github.com/ThinkFlowLab/FlyBot/issues/2079)): `save` and `setDefault` call `fileKindSync` on `workflows/.json` and `workflows/project.json`, still create a missing file and write a plain one, and refuse a link, a hard link or a directory with `workflow/not-a-file`, named once per file and process. The read path asks the same question the write path does ([#2609](https://github.com/ThinkFlowLab/FlyBot/issues/2609)): every read entry point (`open`, `preview`, `bytesOf`, `customize`, `setDefault`, and the stored `default` key `list()` feeds back in) goes through `readDefinition`, which accepts only an id `isWorkflowId` allows, so an id such as `../outside/stolen` answers `workflow/not-found` and no file outside `workflows/` is read. Tests: `core/test/workflow-store.test.js`. | +| `core/src/host/workflow-store.js` | Where a definition lives (#1272 step 3): the presets read from their modules plus the owner's `workflows/.json` in the workspace, and the only writer. `save` validates with the engine's own `validateWorkflow` (the roster's roles are the team; an empty roster is no team to check), refuses a preset id (`workflow/preset-read-only` — *Customize* is the only way to start from one), writes `workflow-doc.js`'s bytes only when they changed, and answers `changed: false` for an unedited definition (acceptance 7). `customize`, `setDefault` (the project's `workflows/project.json`), `list`, `open` (definition + bytes + layout + preview) and `definitions()` (the presets plus the workspace files) round it out; `install` wires `rt.workflowEndpoint` for `workflows` (read) and `workflow-save` (write, in `authz.js`'s `TASK_ENDPOINTS`), so the client writes a file through the host and never to GitHub. A file that does not parse is skipped with one warning instead of breaking the list. The `workflows` folder itself must be a real directory too ([#2057](https://github.com/ThinkFlowLab/FlyBot/issues/2057), [#2068](https://github.com/ThinkFlowLab/FlyBot/issues/2068)): `dirKindSync` from `src/host/real-dir.js` is asked before every listing and read, so a link, a plain file or a check that cannot run lists nothing and opens nothing — `own` is empty, `default` is `null`, `open` answers `workflow/not-found`, `definitions()` answers the presets — which is exactly the answer a missing folder gives, and the linked folder is named once per folder and process in one warning (`ds-bot: workflow folder is not a plain directory …; keeping it empty`). The write side follows the same rule: `save`, `customize` and `setDefault` still create a missing folder as before and refuse a linked one instead of writing into it (`workflow/not-a-folder`), and a workflow file that does not parse is reported once per file and process. The file itself is asked as well ([#2079](https://github.com/ThinkFlowLab/FlyBot/issues/2079)): `save` and `setDefault` call `fileKindSync` on `workflows/.json` and `workflows/project.json`, still create a missing file and write a plain one, and refuse a link, a hard link or a directory with `workflow/not-a-file`, named once per file and process. The read path asks the same question the write path does ([#2609](https://github.com/ThinkFlowLab/FlyBot/issues/2609)): every read entry point (`open`, `preview`, `bytesOf`, `customize`, `setDefault`, and the stored `default` key `list()` feeds back in) goes through `readDefinition`, which accepts only an id `isWorkflowId` allows, so an id such as `../outside/stolen` answers `workflow/not-found` and no file outside `workflows/` is read. A folder that is a real directory but cannot be listed is a refusal, not an empty list ([#2614](https://github.com/ThinkFlowLab/FlyBot/issues/2614)): `list` answers `workflow/unreadable` naming the folder instead of `ok: true` with `own: []`, `save` and `setDefault` still answer `wrote: true` (and `changed`) beside that refusal so a landed write is not hidden, `open` keeps answering a definition by name because reading one name needs only `+x`, and `definitions()` keeps its preset fallback. Tests: `core/test/workflow-store.test.js`. | | `core/src/host/workflow-facts.js` | The fact providers (#1272 step 2, acceptance 10): one function per fact a guard asks about — `github.labels`, `github.checkAtSha`, `github.verdictAtTip`, `github.mergeable`, `github.prBodyTip`, `task.progress`, `manual.tick` — each taking the answer a reader already got and returning one artifact (spec, revision, value), never fetching. No `GITHUB_TOKEN`, no answer, or an answer missing the field is `{ ok: false, reason: 'unreadable', why }`, never a guess: a fact nobody could read refuses the node it feeds instead of passing it. `workflowFact(name, args)` is the one spelling for a caller that has the name as data, and an unknown name is unreadable with the name in the reason. Tests: `core/test/workflow-repo-dev.test.js`. | | `core/src/shared/remote-label.js` | Owner-badge labels for a friend's Bot and “Shared room from …” titles. `friendBotTitle(bot)` / `friendBotOwner(bot)` name a friend's chat from the roster (`Chief (SelfB1233)`) and its owner (`SelfB1233`, the person an approval card names) with one formatter for every client surface (#498); `friendRecord` reads a published roster record (`roster.remoteBots`, name/owner/home at the top level) as well as a chat's `remote`, so the name does not change when the chat appears (#967). `remoteMentionLabel(record, others)` is the one name a mention wears — that title, and on a title two different handles share the home too (`Chief (Bob) · bob2.home`), so the menu, the bubble, the preview and the host's match cannot drift apart (#967). `remoteBotName` / `isAutoOwnerName` / `seatOwnerName` keep group and face-room labels in that format too (#526): Bot Social's ` · ` member label is not a Bot name, and an owner name made up from an install id (`FlyBot 682b8a43`) gives way to the username in the home handle (`Reviewer (flybot682b8a43)`). `isListedRemoteChat` / `listedChatBots` keep gone, hidden, and home-level (`@something.home`) remotes out of the **发给** picker, mentions, and `list_bots` friends lines: a share that ended is not a place to send to. The sidebar asks a different question through `isShownRemoteChat` (#956): a chat whose Bot left the published list keeps its row, marked *no longer shared*, while only a hide the user set (`bot.hidden`) takes a row out. History stays on disk either way. Since #1916 `cleanPeerName` bounds a peer name through the shared `cut` (`shared/text-boundary.js`), so a 60-unit label never ends in half of an emoji. | diff --git a/patches/ds-bot/files/src/host/workflow-store.js b/patches/ds-bot/files/src/host/workflow-store.js index cfc995a79..dca74aa0d 100644 --- a/patches/ds-bot/files/src/host/workflow-store.js +++ b/patches/ds-bot/files/src/host/workflow-store.js @@ -75,14 +75,13 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, const refuseNotFile = (file) => ({ ok: false, code: 'workflow/not-a-file', message: `the workflow file ${file} is not a plain file of its own, so nothing is written there`, problems: [] }) // A directory may not exist yet (a workspace nobody has customized anything in), and a file in it // may be someone's hand-edit that does not parse: one broken file must not take the list down. + // A real folder that cannot be listed is not that case (#2614): `list` turns the failure into a + // refusal, because an empty list here would sit beside the files `open` still reads by name. const ownFiles = () => { if (!folderReal()) return [] - try { - return readdirSync(dir).filter((name) => name.endsWith('.json') && name !== PROJECT_FILE).sort() - } catch { - return [] - } + return readdirSync(dir).filter((name) => name.endsWith('.json') && name !== PROJECT_FILE).sort() } + const refuseUnreadable = () => ({ ok: false, code: 'workflow/unreadable', message: `the workflow folder ${dir} is there but cannot be listed, so its workflows cannot be read`, problems: [] }) const readOwn = (id) => { if (!folderReal()) return null @@ -116,7 +115,13 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, } const list = () => { - const own = ownFiles().flatMap((name) => { + let names + try { + names = ownFiles() + } catch { + return refuseUnreadable() + } + const own = names.flatMap((name) => { const definition = readOwn(name.slice(0, -'.json'.length)) if (definition === null || definition.id === '') { skipped(join(dir, name), name, dir) @@ -141,9 +146,13 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, const { definition, source } = found const target = typeof node === 'string' && node !== '' ? node : definition.nodes.find((entry) => entry.column !== false)?.id ?? definition.nodes[0]?.id ?? null const answer = factsFor({ id: definition.id, node: target }) + const listed = list() return { ok: true, - ...list(), + // A folder that cannot be listed still opens a definition by name (reading one name needs + // only `+x`), so the refusal belongs to the listing half: the listing fields are left out + // rather than spreading an empty `own` over this answer (#2614). + ...(listed.ok ? listed : {}), workflow: { id: definition.id, name: definition.name, @@ -185,11 +194,16 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, return null } })() - if (previous !== bytes) { + const wrote = previous !== bytes + if (wrote) { mkdirSync(dir, { recursive: true }) writeFileSync(path, bytes, { mode: 0o600 }) } - return { ok: true, id: def.id, bytes, changed: previous !== bytes, ...list() } + const listed = list() + // A landed write and a folder that refuses to be listed are both true: saying only `ok: false` + // would hide the write, and `ok: true` beside an empty `own` would hide the folder (#2614). + if (!listed.ok) return { ...listed, id: def.id, wrote, changed: wrote } + return { ok: true, id: def.id, bytes, changed: wrote, ...listed } } const customize = ({ from, id, name } = {}) => { @@ -209,7 +223,9 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, if (!fileWritable(path)) return refuseNotFile(path) mkdirSync(dir, { recursive: true }) writeFileSync(path, `${JSON.stringify({ default: found.definition.id }, null, 2)}\n`, { mode: 0o600 }) - return { ok: true, ...list() } + const listed = list() + if (!listed.ok) return { ...listed, wrote: true, changed: true } + return { ok: true, ...listed } } return { diff --git a/patches/ds-bot/files/test/workflow-store.test.js b/patches/ds-bot/files/test/workflow-store.test.js index a40eaf148..a6596ac92 100644 --- a/patches/ds-bot/files/test/workflow-store.test.js +++ b/patches/ds-bot/files/test/workflow-store.test.js @@ -3,7 +3,7 @@ // actor (acceptance 6 and 7). import assert from 'node:assert/strict' import { createHash } from 'node:crypto' -import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -521,3 +521,84 @@ test('#2609: a stored default that leaves the folder is answered as none', (t) = assert.equal(listed.default, null, 'a stored key is a file name only when it is one') assert.deepEqual(listed.own, []) }) + +// --------------------------------------------------------------------------- +// #2614: a workflow folder that is there but cannot be listed (a restore with the owner bits +// wrong, or a `chmod -R` accident) answered `ok: true` with an empty `own`, while `open` still +// read a definition by name: listing a folder needs `+r`, reading a name in it only `+x`, so the +// two halves disagreed. The listing half refuses with `workflow/unreadable`, a write that landed +// still says `wrote: true`, and getting the permission back lists the files again. +// Root ignores the permission bits, so both cases are skipped there. + +const unlistableFolder = (t) => { + const root = mkdtempSync(join(tmpdir(), 'flybot-workflows-')) + const dir = join(root, 'workflows') + mkdirSync(dir, { recursive: true }) + writeFileSync(join(dir, 'mine.json'), JSON.stringify({ ...blankWorkflow, id: 'mine', name: 'Mine' })) + t.after(() => { + chmodSync(dir, 0o755) + rmSync(root, { recursive: true, force: true }) + }) + chmodSync(dir, 0o311) + return dir +} + +test('#2614: a workflow folder that cannot be listed refuses the list instead of answering empty', (t) => { + if (process.getuid?.() === 0) { + t.skip('root ignores the folder permission bits') + return + } + const dir = unlistableFolder(t) + const store = createWorkflowStore({ dir }) + const listed = store.list() + assert.equal(listed.ok, false) + assert.equal(listed.code, 'workflow/unreadable') + assert.ok(listed.message.includes(`the workflow folder ${dir} is there but cannot be listed`), listed.message) + assert.equal('own' in listed, false, 'a refusal never carries an empty own list') + // Reading a name out of the folder needs only `+x`, so it still answers: that is how the two + // halves disagreed before the refusal existed. + const opened = store.open('mine') + assert.equal(opened.ok, true) + assert.equal(opened.workflow.source, 'workspace') + assert.equal(opened.workflow.definition.name, 'Mine') + // Put the permission back: the same store lists the file again. + chmodSync(dir, 0o755) + const back = store.list() + assert.equal(back.ok, true) + assert.deepEqual(back.own.map((entry) => entry.id), ['mine']) +}) + +test('#2614: a save that landed in an unlistable folder says wrote: true and refuses the list', (t) => { + if (process.getuid?.() === 0) { + t.skip('root ignores the folder permission bits') + return + } + const dir = unlistableFolder(t) + const store = createWorkflowStore({ dir }) + const saved = store.save({ definition: { ...blankWorkflow, id: 'mine', name: 'Mine 2' } }) + assert.equal(saved.ok, false) + assert.equal(saved.code, 'workflow/unreadable') + assert.equal(saved.wrote, true) + assert.equal(saved.changed, true) + assert.equal('own' in saved, false, 'a refusal never carries an empty own list') + assert.equal(JSON.parse(readFileSync(join(dir, 'mine.json'), 'utf8')).name, 'Mine 2', 'the write landed') + // An unedited save writes nothing, and still refuses the list it could not read. + const again = store.save({ definition: store.open('mine').workflow.definition }) + assert.equal(again.ok, false) + assert.equal(again.code, 'workflow/unreadable') + assert.equal(again.wrote, false) + assert.equal(again.changed, false) + // A default lands too, and says the same thing about the listing half. + const set = store.setDefault({ id: 'mine' }) + assert.equal(set.ok, false) + assert.equal(set.code, 'workflow/unreadable') + assert.equal(set.wrote, true) + assert.equal(JSON.parse(readFileSync(join(dir, 'project.json'), 'utf8')).default, 'mine', 'the default landed') + // Reading the default back by name needs only `+x`; the listing half is what refused. + assert.equal(store.open('mine').ok, true) + chmodSync(dir, 0o755) + const back = store.list() + assert.equal(back.ok, true) + assert.equal(back.default, 'mine') + assert.deepEqual(back.own.map((entry) => entry.id), ['mine']) +})