project_state: name a Project State file the reader cannot read

changelog.d/2461-project-state-unreadable.fixed.md

@@ -0,0 +1 @@
+`project_state` no longer reports a Project State file it cannot read as if the folder held none: a `PROJECT.md` that is a folder, or a file the process may not read, now answers `unreadable: PROJECT.md (<reason>)` beside the rows that could be read instead of the listing answering `no Project State files in this folder`, which said there was nothing to read while something was ([#2461](https://github.com/ThinkFlowLab/FlyBot/issues/2461)).

core/src/host/project-state.js

@@ -67,13 +67,20 @@ async function decisionFiles(dir) {
 // is what `project_state` and the board's opened row already answer for a missing one.
 async function fileAt(dir, path) {
   if (isDecisionPath(path) && await dirKind(dirname(join(dir, path))) !== 'real') {
-    throw new Error(`not a plain file: ${path}`)
+    throw notPlainFile(path, true)
   }
   const info = await lstat(join(dir, path))
-  if (!info.isFile()) throw new Error(`not a plain file: ${path}`)
+  if (!info.isFile()) throw notPlainFile(path, info.isSymbolicLink())
   return info
 }
 
+// A name that is there but is not a plain file. `absent` marks the shapes this reader already
+// answers as a missing file — a symlink (#2055) and a decision whose `decisions/` is not a real
+// folder here — so the listing drops them exactly like an absent name. Anything else (a folder, a
+// fifo, a device) is a name the reader cannot read, which the listing answers instead of dropping
+// (#2461): a folder whose `PROJECT.md` is a directory is not a folder with no Project State file.
+const notPlainFile = (path, absent) => Object.assign(new Error(`not a plain file: ${path}`), { absent })
+
 // One Project State file with its byte size, content hash and last-modified time. Exported
 // because the board reads the file a decided meeting archived, and it must refuse exactly what
 // this reader refuses — one rule, one place.
@@ -90,20 +97,40 @@ export async function describeFile(dir, path) {
 // The repository gate belongs to the tool, not to every reader (#1809): a decision file is the
 // board's own artifact and shows up in a folder that is not a repository, while `project_state`
 // keeps refusing such a folder by default. Only the board passes `{ repository: false }`.
+// The reason a listed name has no row, short enough to sit inside one: the system's own errno when
+// it gave one, else the rule the name broke (a folder where a plain file belongs). The name itself
+// stays in the row, so the reason never repeats it and never carries a parenthesis of its own.
+const unreadableReason = (error) => {
+  if (typeof error?.code === 'string' && error.code !== '') return error.code
+  const message = String(error?.message ?? error ?? '').split('\n')[0]
+  return message.startsWith('not a plain file') ? 'not a plain file' : (message.replace(/[()]/g, '') || 'unknown')
+}
+
+// One listed name: a row with its byte size, hash and mtime, or the reason it has none. A name that
+// is absent — or is a link, which this reader reads as absent (#2055) — adds nothing; a name that is
+// there and could not be read is `unreadable: <path> (<reason>)`, never a silent omission (#2461).
+async function listedRow(dir, path) {
+  try {
+    return { file: await describeFile(dir, path) }
+  } catch (error) {
+    if (error?.absent === true || error?.code === 'ENOENT' || error?.code === 'ENOTDIR') return null
+    return { unreadable: { path, reason: unreadableReason(error) } }
+  }
+}
+
 export async function listProjectState(dir, { repository = true } = {}) {
   if (repository && !await repositoryAt(dir)) return { ok: false, reason: `not a repository: ${dir}` }
   const found = []
-  for (const path of PROJECT_STATE_PATHS) {
-    try {
-      found.push(await describeFile(dir, path))
-    } catch { /* an absent Project State file is simply not listed */ }
-  }
-  for (const path of await decisionFiles(dir)) {
-    try {
-      found.push(await describeFile(dir, path))
-    } catch { /* a file that vanished between readdir and read is not listed */ }
+  const unreadable = []
+  const add = async (path) => {
+    const row = await listedRow(dir, path)
+    if (row === null) return
+    if (row.file !== undefined) found.push(row.file)
+    else unreadable.push(row.unreadable)
   }
-  return { ok: true, dir, files: found }
+  for (const path of PROJECT_STATE_PATHS) await add(path)
+  for (const path of await decisionFiles(dir)) await add(path)
+  return { ok: true, dir, files: found, unreadable }
 }
 
 // The bytes of one named Project State file, bounded at `MAX_BYTES`. The path is checked
@@ -145,14 +172,21 @@ export function workDirFor(rt, exec) {
   return rt.teamFolder?.()
 }
 
-const listed = ({ files }) => files.length === 0
+// The answer the tool prints: the rows a caller compares hashes with, and — after them — the names
+// that are there but could not be read. `none` is the answer for a folder with nothing to read,
+// which is exactly what an unreadable name is not (#2461).
+const listed = ({ files, unreadable }) => files.length === 0 && unreadable.length === 0
   ? 'project state: no Project State files in this folder'
-  : ['project state:', ...files.map(file => `${file.path}\t${file.bytes} bytes\tsha256:${file.sha256}\t${file.mtime}`)].join('\n')
+  : [
+      'project state:',
+      ...files.map(file => `${file.path}\t${file.bytes} bytes\tsha256:${file.sha256}\t${file.mtime}`),
+      ...unreadable.map(({ path, reason }) => `unreadable: ${path} (${reason})`),
+    ].join('\n')
 
 export function install(rt) {
   rt.ctx.tools.register(defineTool({
     name: 'project_state',
-    description: 'Read the Project State files of your working folder: PROJECT.md, ROADMAP.md, docs/governance.md, decisions/*.md, LESSONS.md. Without `file` it lists each with its byte size, sha256 content hash and last-modified time, so you can compare the hash with the copy you already have. With `file` it returns that one file, bounded. Answers `absent`, `too large` or `not a repository` with the reason instead of an empty document.',
+    description: 'Read the Project State files of your working folder: PROJECT.md, ROADMAP.md, docs/governance.md, decisions/*.md, LESSONS.md. Without `file` it lists each with its byte size, sha256 content hash and last-modified time, so you can compare the hash with the copy you already have. With `file` it returns that one file, bounded. Answers `absent`, `too large` or `not a repository` with the reason instead of an empty document, and a listed name that is there but cannot be read as `unreadable: <path> (<reason>)` rather than leaving it out.',
     parameters: {
       file: { type: 'string', description: 'Optional Project State path to read, e.g. decisions/x.md; omit to list them all' },
     },

core/test/project-state-unreadable.test.js

@@ -0,0 +1,114 @@
+// A Project State file that is there but cannot be read must not be dropped on the floor. The
+// reader already answers `not a repository`, `absent` and `too large` with the reason instead of
+// an empty document; a listed name that exists and fails to read is the same kind of answer.
+// Today `listProjectState` catches every failure and lists nothing, so a folder whose PROJECT.md
+// is a folder, or is unreadable, reads as `no Project State files in this folder` — the one
+// answer that says there is nothing to read. The name and the reason belong in the answer:
+// `unreadable: PROJECT.md (<code>)`, with the name written the way the other rows write it
+// (relative to the working folder, because the answer can be pasted into a public issue).
+//
+// The control is a folder with no Project State file at all, which must keep answering exactly
+// `no Project State files` — an answer that says "nothing here" is wrong only when something is.
+import assert from 'node:assert/strict'
+import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { test } from 'node:test'
+import { apply } from '../index.js'
+import { fakeContext } from './fake-ctx.js'
+
+const TEAM = {
+  version: 1, mainBotIds: ['a'],
+  bots: { a: { id: 'a', name: 'Chief', role: 'Boss', sessionId: 'a', createdAt: 1 } },
+  rooms: {}, agents: {}, exchanges: [], exchangeTurns: {}, questions: {}, prefs: {}, revision: 0,
+}
+
+const NO_FILES = 'project state: no Project State files in this folder'
+// The rows this reader answers with one per line; `unreadable` is the one this test is about.
+const rows = listing => listing.split('\n').slice(1)
+const unreadable = listing => rows(listing).filter(line => line.startsWith('unreadable: '))
+
+// A folder the reader calls a repository, with nothing in it that this reader lists. The test
+// writes the one state file it wants, so the red case is the only thing in the folder.
+function emptyRepository() {
+  const dir = mkdtempSync(join(tmpdir(), 'ds-bot-project-state-unreadable-'))
+  mkdirSync(join(dir, '.git'))
+  return dir
+}
+
+async function boot({ dir } = {}) {
+  const home = mkdtempSync(join(tmpdir(), 'ds-bot-project-state-unreadable-home-'))
+  writeFileSync(join(home, 'state.json'), JSON.stringify(TEAM))
+  const ctx = fakeContext()
+  Object.assign(ctx, {
+    llm: { listProviders: () => [], listModels: async () => [], resolveModelInfo: async () => ({}) },
+    workspaceRegistry: { get: () => (dir === undefined ? undefined : { path: dir }), resolveByPath: async () => undefined, create: async path => ({ id: 'w1', path }), archiveSession: async () => {} },
+    sessionController: { create: async () => ({ sessionId: 's1' }), resolveAgent: async id => ({ agent: { session: { id }, id, status: 'idle', cancel() {} } }), selectModel: async () => {}, page: async () => ({ records: [], hasMore: false }) },
+    sessionTitle: { rename: async () => {} },
+    sessions: { flush: async () => {} },
+  })
+  const rt = apply(ctx, { home })
+  await ctx.flybot.ready()
+  const exec = { agent: { id: 'a', session: { header: { cwd: dir } } } }
+  const tool = args => ctx.registeredTools.get('project_state').execute(args, exec)
+  const stop = async () => { await ctx.dispose(); rmSync(home, { recursive: true, force: true, maxRetries: 5 }) }
+  return { ctx, rt, home, tool, stop }
+}
+
+test('#2461: a Project State name that is a folder is answered as unreadable, never as no files', async (t) => {
+  const dir = emptyRepository()
+  mkdirSync(join(dir, 'PROJECT.md'))
+  const { tool, stop } = await boot({ dir })
+  t.after(async () => { await stop(); rmSync(dir, { recursive: true, force: true, maxRetries: 5 }) })
+
+  const listing = await tool({})
+  assert.notEqual(listing, NO_FILES, 'the folder is not empty: one listed name is there and could not be read')
+  assert.equal(listing.split('\n')[0], 'project state:', 'the answer still opens as the listing it is')
+  assert.deepEqual(unreadable(listing).map(line => line.replace(/ \([^)]+\)$/, '')), ['unreadable: PROJECT.md'],
+    'the name is written the way the rows write it, relative to the working folder')
+  for (const line of unreadable(listing)) assert.match(line, /^unreadable: PROJECT\.md \([^)\n]+\)$/,
+    'the reason travels with the name, so a reader knows it is not an absent file')
+})
+
+test('#2461: a Project State file that cannot be read keeps its name and reason beside the rows that can', async (t) => {
+  const dir = emptyRepository()
+  mkdirSync(join(dir, 'PROJECT.md'))
+  writeFileSync(join(dir, 'ROADMAP.md'), '# Roadmap\n')
+  const { tool, stop } = await boot({ dir })
+  t.after(async () => { await stop(); rmSync(dir, { recursive: true, force: true, maxRetries: 5 }) })
+
+  const listing = await tool({})
+  const roadmap = rows(listing).find(line => line.startsWith('ROADMAP.md\t'))
+  assert.ok(roadmap, 'the file that can be read is still listed, with its size, hash and mtime')
+  assert.equal(roadmap.split('\t').length, 4)
+  assert.deepEqual(unreadable(listing).length, 1, 'and the one that cannot be read is named once, not swallowed')
+})
+
+// A root process reads a chmod 000 file, so this case is declared skipped rather than run there —
+// the shape `core/test/diagnostics.test.js` already uses for its own root-only cases.
+test('#2461: a state file the process may not read answers its code, not an empty folder', {
+  skip: process.getuid?.() === 0 ? 'root reads a chmod 000 file' : undefined,
+}, async (t) => {
+  const dir = emptyRepository()
+  writeFileSync(join(dir, 'PROJECT.md'), '# Project\n')
+  chmodSync(join(dir, 'PROJECT.md'), 0o000)
+  const { tool, stop } = await boot({ dir })
+  t.after(async () => {
+    await stop()
+    chmodSync(join(dir, 'PROJECT.md'), 0o600)
+    rmSync(dir, { recursive: true, force: true, maxRetries: 5 })
+  })
+
+  const listing = await tool({})
+  assert.notEqual(listing, NO_FILES, 'an unreadable file is not an absent one')
+  assert.match(listing, /(^|\n)unreadable: PROJECT\.md \(EACCES\)($|\n)/,
+    'the errno is what tells a reader to fix permissions rather than look for another file')
+})
+
+test('#2461: a folder with no Project State file still answers exactly no files', async (t) => {
+  const dir = emptyRepository()
+  const { tool, stop } = await boot({ dir })
+  t.after(async () => { await stop(); rmSync(dir, { recursive: true, force: true, maxRetries: 5 }) })
+
+  assert.equal(await tool({}), NO_FILES)
+})