fix(host): refuse a workflow folder that cannot be listed

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)).

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 {

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'])
+})